Skip to content

Conversation

@XuNeo
Copy link
Contributor

@XuNeo XuNeo commented Feb 2, 2025

Note: Please adhere to Contributing Guidelines.

Summary

 Use Inode.__repr__ for print and use single line for all information.
    Add 'verbose' and 'nodetype' option to show more details and only print specifiy node type like PIPE.
    Add / to name for pseudodir.
    Catch memory error when accessing node element.
    
    E.g.
    
    (gdb) foreach inode --nodetype pipe
            ├── run/ 0x41a27a50 PSEUDODIR
            │   ├── bt:bluetoothCS1fd26 0x4530c510 PIPE
            │   ├── bt:bluetoothCS20 0x42daae40 PIPE
            │   ├── socketpair0x457fd2e0SC20540 0x44ea27b0 PIPE
            │   └── tmp/ 0x42dd7cf0 PSEUDODIR
            │       │   └── usock/ 0x42dd6c10 PSEUDODIR
            │       │       └── speech.usockHD 0x42dc8270 PIPE
            │       ├── central_lite.socketCS5e 0x42e1e7b0 PIPE
            │       ├── central_service_lite.socketSC86 0x432ee6d0 PIPE
            │       │   └── usock/ 0x43d6ed60 PSEUDODIR
            │       │       └── speech.usockHD 0x43da2ba0 PIPE
            │       ├── uv-feature-sockSC8d 0x43540430 PIPE
            │       ├── uv-miot-sockCS54 0x42e272e0 PIPE
            │       └── uv-miot-sockSC54 0x42e270b0 PIPE

Impact

New command option and code refactor.

Testing

Tested locally with a coredump.

Instead of storing it to self

Signed-off-by: xuxingliang <xuxingliang@xiaomi.com>
Use Inode.__repr__ for print and use single line for all information.
Add 'verbose' and 'nodetype' option to show more details and only print specifiy node type like PIPE.
Add / to name for pseudodir.
Catch memory error when accessing node element.

E.g.

(gdb) foreach inode --nodetype pipe
        ├── run/ 0x41a27a50 PSEUDODIR
        │   ├── bt:bluetoothCS1fd26 0x4530c510 PIPE
        │   ├── bt:bluetoothCS20 0x42daae40 PIPE
        │   ├── socketpair0x457fd2e0SC20540 0x44ea27b0 PIPE
        │   └── tmp/ 0x42dd7cf0 PSEUDODIR
        │       │   └── usock/ 0x42dd6c10 PSEUDODIR
        │       │       └── speech.usockHD 0x42dc8270 PIPE
        │       ├── central_lite.socketCS5e 0x42e1e7b0 PIPE
        │       ├── central_service_lite.socketSC86 0x432ee6d0 PIPE
        │       │   └── usock/ 0x43d6ed60 PSEUDODIR
        │       │       └── speech.usockHD 0x43da2ba0 PIPE
        │       ├── uv-feature-sockSC8d 0x43540430 PIPE
        │       ├── uv-miot-sockCS54 0x42e272e0 PIPE
        │       └── uv-miot-sockSC54 0x42e270b0 PIPE

Signed-off-by: xuxingliang <xuxingliang@xiaomi.com>
@github-actions github-actions bot added Area: Tooling Size: M The size of the change in this PR is medium labels Feb 2, 2025
@nuttxpr
Copy link

nuttxpr commented Feb 2, 2025

[Experimental Bot, please feedback here]

This PR description, while providing some information, is insufficient for a proper NuttX review. Here's a breakdown of its shortcomings and how to improve it:

Summary - Needs More Detail:

  • Vague Description: "Use Inode.repr" doesn't explain why this change is beneficial. Is it for readability, debugging, consistency, or some other reason?
  • Missing Context: What is the foreach inode command? Is this a GDB command? This needs clarification for reviewers unfamiliar with the specific debugging context.
  • Incomplete Explanation: "Catch memory error" is good, but how are these errors caught and handled? What was the previous behavior (likely a crash)?
  • Example Over Explanation: The example is helpful, but it replaces a clear explanation of the changes. The summary should concisely describe the changes before showing the example.

Impact - Insufficient Detail:

  • "New command option and code refactor" is too broad. Specify which command is affected. Mention if the refactor has performance implications or changes any existing behavior.
  • Address all impact points: Even if the answer is "NO", explicitly state it for all impact categories (user, build, hardware, documentation, security, compatibility). If "YES," provide details. For example, even if there's no user impact during runtime, the new command options change how users interact with the debugger, which should be mentioned.

Testing - Woefully Inadequate:

  • "Tested locally with a coredump" is not enough. What specific tests were run? What were the expected results?
  • No "before" and "after" logs: The template clearly requests these. Provide actual logs, not just placeholders. Show how the output changes with the new options. The example in the summary could be part of the "after" logs, but it needs to be clearly labeled and ideally show different scenarios (using --verbose, --nodetype, etc.).
  • Missing System Information: Provide details about your build host and target as requested: OS, CPU architecture, compiler, NuttX version, board configuration, etc. This information is critical for reproducing your results.

Revised PR Description (Example):

Summary

This PR improves the foreach inode GDB command (used for inspecting inodes in a NuttX coredump) by providing more detailed and flexible output. It uses the Inode.__repr__ method for consistent formatting and provides a single-line summary of each inode. Two new options are added:

  • --verbose: Displays more detailed information about each inode.
  • --nodetype <type>: Filters the output to display only inodes of the specified type (e.g., PIPE, SOCKET).

Additionally, pseudo-directories are now visually distinguished by appending a / to their names. Error handling is improved by catching potential memory errors when accessing inode elements; previously, these errors could lead to a GDB crash.

Related Issue: [NuttX #1234](replace with actual issue if applicable)

Impact

  • New feature added: Yes, the --verbose and --nodetype options are added to the foreach inode GDB command.
  • Impact on user: YES. Users debugging NuttX with GDB will have access to new options for inspecting inodes.
  • Impact on build: NO.
  • Impact on hardware: NO.
  • Impact on documentation: YES. The GDB command documentation should be updated to reflect the new options. (Documentation updates are included in this PR).
  • Impact on security: NO.
  • Impact on compatibility: NO.
  • Anything else to consider: This change improves the debuggability of NuttX.

Testing

I confirm that changes are verified on my local setup and work as intended:

  • Build Host: Linux (Ubuntu 22.04), x86_64, GCC 11.2.0
  • Target: sim:qemu-arm

Testing logs before change:

(gdb) foreach inode
<previous output here>  <-- Show the actual output before your changes

Testing logs after change:

(gdb) foreach inode
├── run/ 0x41a27a50 PSEUDODIR
│   ├── ... (other inodes)

(gdb) foreach inode --nodetype pipe
├── run/ 0x41a27a50 PSEUDODIR
│   ├── bt:bluetoothCS1fd26 0x4530c510 PIPE
│   ├── ... (other pipes)

(gdb) foreach inode --verbose
├── run/ 0x41a27a50 PSEUDODIR (permissions: ..., size: ...) <--- Example verbose output
│   ├── ... (other inodes with verbose details)

By providing more detail and context, you make it easier for reviewers to understand and approve your PR. Remember to replace the placeholders with your actual information.

@jerpelea jerpelea merged commit 8995e5a into apache:master Feb 3, 2025
39 checks passed
@XuNeo XuNeo deleted the nxgdb-fs-simpifiy-inode-print branch February 3, 2025 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: Tooling Size: M The size of the change in this PR is medium

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants