Skip to content

Conversation

@JianyuWang0623
Copy link
Contributor

@JianyuWang0623 JianyuWang0623 commented Oct 17, 2024

Summary

Add support for reading from/writing to standard input/output.
The feature depends on #2737

Impact

nshlib/cmd_dd

Testing

General tests

# 1. Input from stdin and output to stdout
#   Keyboard input: 12345AAAAABBBBB
nsh> dd bs=5
1234512345AAAAAAAAAABBBBBBBBBB

# 2. Input from file and output to stdout
nsh> dd if=/etc/init.d/rc.sysinit
mkrd -m 2 -s 512 1024
mkfatfs /dev/ram2
mount -t vfat /dev/ram2 "/tmp"

# 3. Input from stdin and output to file
#    Keyboard input: QWERT
dd of=/data/dd_stdout bs=5
#    Then, cat the output file in host (based on HostFS):
$ cat ../nuttx/dd_stdout
QWERT

@nuttxpr
Copy link

nuttxpr commented Oct 17, 2024

[Experimental Bot, please feedback here]

The PR summary is missing crucial information:

  • Why: It lacks a clear explanation of why adding standard input/output support to nshlib/cmd_dd is necessary. What specific use cases or limitations does this address?
  • What: While it mentions nshlib/cmd_dd, it doesn't detail how the change works within the code.
  • Issue References: Are there related issues in the NuttX or NuttX Apps repositories? If so, provide links.

Impact:

  • The impact section is overly brief. You need to answer all the questions with more detail than just "nshlib/cmd_dd". For example:
    • New Feature?: Yes, this adds standard input/output redirection.
    • User Impact?: YES - Users can now use pipes and redirection with the dd command in nsh. Explain how this might change existing workflows.
    • Build Impact?: Possibly. Does the dependency on nuttx-apps PR Add pipeline support for nsh commandline #2737 introduce any build requirements or changes to the build process?
    • Hardware Impact: Unclear. Does this depend on specific hardware features or configurations (like PIPES)?
    • Documentation Impact: YES - Documentation needs to be updated to describe the new input/output capabilities of the dd command.
    • Security/Compatibility Impact: Consider if these changes have any security implications or affect backward compatibility.

Testing:

  • Insufficient Detail:
    • Build Host(s): Provide specifics about your development environment: OS, CPU architecture, compiler version.
    • Target(s): Specify the exact architecture (e.g., sim:qemu-arm) and board configurations used for testing.
  • Testing Logs: Include the actual logs from running the tests both before and after your changes. This helps reviewers verify the functionality and identify potential regressions.

In summary, this PR does not meet the NuttX requirements in its current state. You need to provide more context, thoroughly address the impact assessment, and include detailed testing information.

Test
  1. Input from stdin and output to stdout
       Keyboard input: 12345AAAAABBBBB
    nsh> dd bs=5
    1234512345AAAAAAAAAABBBBBBBBBB

  2. Input from file and output to stdout
    nsh> dd if=/etc/init.d/rc.sysinit
    mkrd -m 2 -s 512 1024
    mkfatfs /dev/ram2
    mount -t vfat /dev/ram2 "/tmp"

  3. Input from stdin and output to file
       Keyboard input: QWERT
    dd of=/data/dd_stdout bs=5

    Then, cat the output file in host (based on HostFS):
    $ cat ./dd_stdout
    QWERT

Signed-off-by: wangjianyu3 <wangjianyu3@xiaomi.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants