[BREAKING] system/dd: using system dd to instead nsh dd, avoid duplicate code

[Donny9]

Summary

system/dd: using system dd to instead nsh dd, avoid duplicate code, and keep function align with nsh dd.

Impact

remove dd config: NSH_DISABLE_DD/NSH_CMDOPT_DD_STATS
enable SYSTEM_DD when !DEFAULT_SMALL(keep dd cmd always enable)

Testing

sim:nsh

[nuttxpr]

[Experimental Bot, please feedback here]

No, this PR summary does not adequately meet the NuttX requirements. Here's why and how to fix it:

• Insufficient Detail in Summary: "using system dd to instead nsh dd, avoid duplicate code, and keep function align with nsh dd" is too vague.

  • Why is duplicate code a problem (maintenance, size, bugs)?
  • What part of the code is changing (files, functions, modules)? Be specific! "system/dd" is a start, but mention specific files.
  • How does it work? Does this mean the dd command in NSH now calls a system-level dd utility? Explain the mechanism.

• Missing Issue References: Are there any related NuttX or NuttX Apps issues that this PR addresses? If so, link them. If not, explicitly state "None."

• Incomplete Impact Assessment:

  • Impact on user: Does this change the behavior of the dd command in any way that a user would notice? Even if you think the answer is no, explain why. For example, "No, the user interface and functionality of the dd command remain the same."
  • Impact on build: You mentioned config changes. Describe precisely what changes to the configuration are required. Which Kconfig files are affected? How should users update their .config files? Provide examples.
  • Impact on hardware: You've only mentioned "sim:nsh." Does this change affect any physical hardware? Even if it's platform-agnostic, state that explicitly. "No, this change affects only the NSH implementation of dd and is independent of the underlying hardware."
  • Impact on documentation: Does any documentation need updating? If not, state that. "No, this change does not require documentation updates as the user interface and functionality of the dd command are unchanged."
  • Impact on security, compatibility: Even if the answer is "NO" to these, state it explicitly and briefly justify why. For example, "No security impact, no changes to data handling or access." "Backward compatible, the dd command's interface and functionality are unchanged."

• Insufficient Testing Information: "sim:nsh" is far too brief.

  • Build Host: Be specific about your development environment. Example: "Linux, Ubuntu 22.04, x86_64, GCC 11.2.0"
  • Target: Specify simulator details. Example: "qemu-system-arm, Cortex-M4F"
  • Testing logs: You haven't provided any logs. Show examples of the dd command being used before and after the change, demonstrating that the behavior is the same. Even simple examples are better than none.

Example of improved summary:

## Summary

This PR refactors the NSH `dd` command to utilize a common system-level `dd` implementation located in `system/dd/dd.c`. This eliminates duplicate code previously present in the NSH implementation (`apps/nshlib/nsh_ddcmd.c`), reducing code size and maintenance overhead. The functionality of the `dd` command remains unchanged, ensuring consistency and preventing regressions.  This addresses the code duplication issue highlighted in [NuttX Issue #1234](replace with actual issue link if one exists, otherwise state "None").

## Impact

* **Impact on user:** No. The user interface and functionality of the `dd` command are unchanged.
* **Impact on build:** Yes. The `NSH_DISABLE_DD` and `NSH_CMDOPT_DD_STATS` Kconfig options are removed.  `SYSTEM_DD` is now enabled whenever `DEFAULT_SMALL` is disabled, ensuring that the `dd` command is always available. Users should remove these deprecated options from their `.config` files. In `apps/Kconfig`, remove the lines related to `NSH_DISABLE_DD` and `NSH_CMDOPT_DD_STATS`.
* **Impact on hardware:** No.  This change is independent of the underlying hardware. It only refactors the NSH implementation of the `dd` command.
* **Impact on documentation:** No.  The user interface and functionality remain unchanged.
* **Impact on security:** No.  No changes to data handling or access.
* **Impact on compatibility:** Backward compatible. The `dd` command's interface and functionality are unchanged.

## Testing

* **Build Host:** Linux, Ubuntu 22.04, x86_64, GCC 11.2.0
* **Target:** qemu-system-arm, Cortex-M4F (sim:nsh)

**Testing logs before change:**

nsh> dd if=/dev/zero of=/dev/null bs=512 count=10
10+0 records in
10+0 records out
5120 bytes transferred in 0.000171 secs (29941520 bytes/sec)
nsh>

**Testing logs after change:**

nsh> dd if=/dev/zero of=/dev/null bs=512 count=10
10+0 records in
10+0 records out
5120 bytes transferred in 0.000168 secs (30476190 bytes/sec)
nsh>

By providing more specific information, you significantly increase the chances of your PR being reviewed and merged quickly. Remember to replace the example details with your actual information.

[lupyuen]

We'll change Draft Mode to Ready For Review, after voting has closed. Thanks :-)

• [BREAKING] boards/defconfig: remove unused config about dd nuttx#16198 (comment)

[cederom]

Thanks @Donny9 :-) My remarks below (marked as RC until discussion / vote is resolved on the mailing list):

• Is it really necessary to remove built-in nsh/dd? What are the benefits over breaking build compatibility with os and apps?
• It is impossible to keep both (i.e. nsh/dd as minimalistic and apps/system/dd as fully featured) as exclusive selections?
• Why not extend nsh/dd with new features while keep the minimal as default?
• Aside question did NuttX consider /usr/bin/dd versus built-in dd (something like built-in time versus /usr/bin/time in Unix)?
• Documentation needs an update in either case :-P

[acassis]

@cederom @lupyuen @Donny9 comparing both file with meld I can see they are basically the same code:

alan@dev:~/nuttxspace/apps$ meld nshlib/nsh_ddcmd.c system/dd/dd_main.c

But I think system/dd will evolve to be more complete and compatible with Linux/Unix over the time.

For example, nsh_ddcmd.c has CONFIG_NSH_CMDOPT_DD_STATS that doesn't exist in system/dd, so it rings a bell that system/dd is more focused on compatibility vs code size.

I think that CONFIG_NSH_CMDOPT_DD_STATS could be renamed to CONFIG_SYSTEM_DD_MINIMUM and included in other parts there that aren't essential for a simple dd command.

[pussuw]

This causes a regression when NSH_FILEAPPS=y, as the full path to /bin/dd must be provided. I'm okay with this, since I put /bin into PATH, but others might not. Just something to keep in mind.

[Donny9]

@cederom @lupyuen @Donny9 comparing both file with meld I can see they are basically the same code:

alan@dev:~/nuttxspace/apps$ meld nshlib/nsh_ddcmd.c system/dd/dd_main.c

But I think system/dd will evolve to be more complete and compatible with Linux/Unix over the time.

For example, nsh_ddcmd.c has CONFIG_NSH_CMDOPT_DD_STATS that doesn't exist in system/dd, so it rings a bell that system/dd is more focused on compatibility vs code size.

I think that CONFIG_NSH_CMDOPT_DD_STATS could be renamed to CONFIG_SYSTEM_DD_MINIMUM and included in other parts there that aren't essential for a simple dd command.

Done~ rename CONFIG_NSH_CMDOPT_DD_STATS to CONFIG_SYSTEM_DD_STATS.

[lupyuen]

@Donny9 Please close the voting. Thanks!
This Breaking PR compiles with the NuttX Breaking Changes Handling Process:

• Special CI Checks are successful
• NuttX Dashboard for This PR
• Tested OK with OSTest on Real Hardware: Avaota-A1 A527 Arm64 SBC and Oz64 SG2000 64-bit RISC-V SBC
• No Blocking Binding Vote
• Please respond to the comments by @cederom: [BREAKING] system/dd: using system dd to instead nsh dd, avoid duplicate code #3057 (review)
• When @cederom has Approved the PR: Please merge NuttX Apps PR, followed by NuttX Kernel PR. That's because: When we merge the NuttX Kernel PR, it will trigger the Daily Build. Which will fail unless NuttX Apps PR has already been merged.
  

[Donny9]

Thanks @Donny9 :-) My remarks below (marked as RC until discussion / vote is resolved on the mailing list):

• Is it really necessary to remove built-in nsh/dd? What are the benefits over breaking build compatibility with os and apps?

@cederom Firstly, we need to ensure that only one implementation of dd is retained in NuttX, because having two implementations would increase complexity for developers in terms of choosing which one to use. Moreover, if there are two versions, it could lead to misalignment in dd functionality, resulting in more maintenance work and complaints.

I understand that the original reason for introducing the built-app version of dd was that it allowed for independent stack space settings, making it convenient to observe the stack consumption by the file system(If the stack consumed by dd is excessively large, it will result in a larger stack for nsh, thereby reducing the available memory for the system. Therefore, a built-app approach would be preferable).

In reality, there is no significant difference for developers between the built-app dd and the command-line dd. Furthermore, there are indeed many other commands in the system that exist as built-apps, such as tar, ssh, scp, sb, rb, etc.

It is impossible to keep both (i.e. nsh/dd as minimalistic and apps/system/dd as fully featured) as exclusive selections?
Why not extend nsh/dd with new features while keep the minimal as default?

As mentioned above, having two implementations of dd is highly undesirable. As for the extent to which the simplified version of dd lacks functionality compared to the full version, it is difficult to gauge.

Aside question did NuttX consider /usr/bin/dd versus built-in dd (something like built-in time versus /usr/bin/time in Unix)?

In my opinion, their main differences are as follows:

1. The startup methods differ. In the former case, after parsing via nsh_parse, dd is executed directly through a function call. In the latter case, a new task is initiated via posix_spawn, and the corresponding executable binary is executed to accomplish the functionality of dd.
2. The execution contexts differ. The former operates within the context of nsh, while the latter runs as an independent new task.

• Documentation needs an update in either case :-P

Done

[cederom]

Thank you @Donny9 :-)

Happy Easter :-) :-)