Skip to content

sim_hostfs:add host_errno_convert API for convert result #15552

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 15, 2025

Conversation

crafcat7
Copy link
Contributor

Summary

This PR comes from the discussion of #15535 (comment).
A common conversion interface host_errno_convert is provided in arch/sim/posix. Subsequent contributors with needs can add host to nuttx errno conversion here

Impact

Modified all places where errno is returned in arch/sim/posix to be converted via host_errno_convert before returning (current implementation is to return directly)

Testing

Build Host(s): Linux x86
Target(s): sim/nsh

nsh> cmocka_fs_test
[==========] nuttx_fs_test_suites: Running 41 test(s).
[ RUN      ] test_nuttx_fs_creat01
[       OK ] test_nuttx_fs_creat01
[ RUN      ] test_nuttx_fs_dup01
[       OK ] test_nuttx_fs_dup01
[ RUN      ] test_nuttx_fs_fcntl01
[       OK ] test_nuttx_fs_fcntl01
[ RUN      ] test_nuttx_fs_fstat01
[       OK ] test_nuttx_fs_fstat01
[ RUN      ] test_nuttx_fs_fstatfs01
[       OK ] test_nuttx_fs_fstatfs01
[ RUN      ] test_nuttx_fs_fsync01
[       OK ] test_nuttx_fs_fsync01
[ RUN      ] test_nuttx_fs_fsync02
the fbsize = 4096,buffer size=4096
[       OK ] test_nuttx_fs_fsync02
[ RUN      ] test_nuttx_fs_getfilep01
[       OK ] test_nuttx_fs_getfilep01
[ RUN      ] test_nuttx_fs_mkdir01
[       OK ] test_nuttx_fs_mkdir01
[ RUN      ] test_nuttx_fs_open01
[       OK ] test_nuttx_fs_open01
[ RUN      ] test_nuttx_fs_open02
[       OK ] test_nuttx_fs_open02
[ RUN      ] test_nuttx_fs_opendir01
[       OK ] test_nuttx_fs_opendir01
[ RUN      ] test_nuttx_fs_opendir02
[       OK ] test_nuttx_fs_opendir02
[ RUN      ] test_nuttx_fs_pread01
[       OK ] test_nuttx_fs_pread01
[ RUN      ] test_nuttx_fs_pwrite01
[       OK ] test_nuttx_fs_pwrite01
[ RUN      ] test_nuttx_fs_readdir01
[       OK ] test_nuttx_fs_readdir01
[ RUN      ] test_nuttx_fs_readlink01
[       OK ] test_nuttx_fs_readlink01
[ RUN      ] test_nuttx_fs_rename01
[       OK ] test_nuttx_fs_rename01
[ RUN      ] test_nuttx_fs_rename02
[       OK ] test_nuttx_fs_rename02
[ RUN      ] test_nuttx_fs_rewinddir01
[       OK ] test_nuttx_fs_rewinddir01
[ RUN      ] test_nuttx_fs_rmdir01
[       OK ] test_nuttx_fs_rmdir01
[ RUN      ] test_nuttx_fs_rmdir02
[       OK ] test_nuttx_fs_rmdir02
[ RUN      ] test_nuttx_fs_rmdir03
[       OK ] test_nuttx_fs_rmdir03
[ RUN      ] test_nuttx_fs_seek01
[       OK ] test_nuttx_fs_seek01
[ RUN      ] test_nuttx_fs_seek02
[       OK ] test_nuttx_fs_seek02
[ RUN      ] test_nuttx_fs_statfs01
[       OK ] test_nuttx_fs_statfs01
[ RUN      ] test_nuttx_fs_symlink01
[       OK ] test_nuttx_fs_symlink01
[ RUN      ] test_nuttx_fs_truncate01
[       OK ] test_nuttx_fs_truncate01
[ RUN      ] test_nuttx_fs_unlink01
[       OK ] test_nuttx_fs_unlink01
[ RUN      ] test_nuttx_fs_write01
[       OK ] test_nuttx_fs_write01
[ RUN      ] test_nuttx_fs_write02
[       OK ] test_nuttx_fs_write02
[ RUN      ] test_nuttx_fs_write03
[       OK ] test_nuttx_fs_write03
[ RUN      ] test_nuttx_fs_append01
[       OK ] test_nuttx_fs_append01
[ RUN      ] test_nuttx_fs_sendfile01
[       OK ] test_nuttx_fs_sendfile01
[ RUN      ] test_nuttx_fs_sendfile02
[       OK ] test_nuttx_fs_sendfile02
[ RUN      ] test_nuttx_fs_stream01
[       OK ] test_nuttx_fs_stream01
[ RUN      ] test_nuttx_fs_stream02
[       OK ] test_nuttx_fs_stream02
[ RUN      ] test_nuttx_fs_stream03
[       OK ] test_nuttx_fs_stream03
[ RUN      ] test_nuttx_fs_stream04
[       OK ] test_nuttx_fs_stream04
[ RUN      ] test_nuttx_fs_eventfd
[       OK ] test_nuttx_fs_eventfd
[ RUN      ] test_nuttx_fs_poll01
[       OK ] test_nuttx_fs_poll01
[==========] nuttx_fs_test_suites: 41 test(s) run.
[  PASSED  ] 41 test(s).

@github-actions github-actions bot added Arch: simulator Issues related to the SIMulator Size: S The size of the change in this PR is small labels Jan 15, 2025
@nuttxpr
Copy link

nuttxpr commented Jan 15, 2025

[Experimental Bot, please feedback here]

Yes, this PR appears to meet the NuttX requirements, although it could be more thorough.

Strengths:

  • Clear Summary: The summary explains the "why," "what," and "how" of the change, and links back to the originating discussion.
  • Impact Assessment: Addresses several impact areas, correctly identifying most as "NO."
  • Testing Provided: Includes a test log, demonstrating that testing was performed.

Weaknesses and Suggestions for Improvement:

  • Incomplete Impact Assessment: While several impact areas are marked "NO," they should still have brief explanations justifying that assessment (e.g., "Impact on build: NO - This change only affects the sim target and doesn't modify the build process itself."). This helps reviewers quickly understand the reasoning.
  • Missing "Before" Test Log: The "Testing logs before change" section is empty. This makes it impossible to determine if the change had the desired effect. Include the output of the test run before applying the changes.
  • Limited Testing Scope: While cmocka_fs_test is a good start, consider broader testing. The change affects errno handling, so tests that specifically exercise error conditions (e.g., trying to open a non-existent file) would be beneficial. Also, consider testing different configurations of the sim target.
  • Missing Detail in Testing: The testing section only lists the OS and target architecture. Specify the full NuttX configuration used for testing (e.g., sim:nsh). Include the compiler version and any relevant build flags.

Specific Recommendations:

  • Impact on user: Explain why there's no user impact. Even if the change is internal, it's helpful to articulate why users won't notice a difference.
  • Impact on hardware: Clarify why there's no hardware impact. Specifically mention that this change only applies to the simulator.
  • Impact on documentation: If no documentation updates are needed, explicitly state that and the reason (e.g., "Impact on documentation: NO - This change is internal and does not affect the public API.").
  • Impact on security: Even if there's no perceived security impact, briefly justify it (e.g., "Impact on security: NO - This change only affects error code mapping and doesn't introduce any new vulnerabilities.").
  • Impact on compatibility: Same as security, justify the "NO" (e.g., "Impact on compatibility: NO - This change is isolated to the sim target and does not affect compatibility with other targets or previous versions of NuttX.").

By addressing these points, the PR will be more complete and easier for reviewers to evaluate. A more complete PR will increase the likelihood of it being merged quickly.

@xiaoxiang781216 xiaoxiang781216 requested a review from yamt January 15, 2025 03:14
@xiaoxiang781216
Copy link
Contributor

@yamt could you review the change match your expect?

@yamt
Copy link
Contributor

yamt commented Jan 15, 2025

  • i suspect the "generic" error number used for the case where conversion is not available might need to be per-operation.
  • what's your plan for other backends? (eg. arch/xtensa/src/common/xtensa_hostfs.c)
  • can you add a version for #ifndef CONFIG_HOST_LINUX which converts all errors to a generic one, eg. EIO?

@xiaoxiang781216
Copy link
Contributor

  • i suspect the "generic" error number used for the case where conversion is not available might need to be per-operation.

it's better to apply the complex approach util we hit the problem you describe.

  • what's your plan for other backends? (eg. arch/xtensa/src/common/xtensa_hostfs.c)

apply the similar code to xtensa_hostfs.c.

  • can you add a version for #ifndef CONFIG_HOST_LINUX which converts all errors to a generic one, eg. EIO?

OK, if you want.

@xiaoxiang781216 xiaoxiang781216 merged commit 118f827 into apache:master Jan 15, 2025
14 checks passed
@yamt
Copy link
Contributor

yamt commented Jan 16, 2025

  • i suspect the "generic" error number used for the case where conversion is not available might need to be per-operation.

it's better to apply the complex approach util we hit the problem you describe.

do you mean s/apply/avoid/ ?

  • what's your plan for other backends? (eg. arch/xtensa/src/common/xtensa_hostfs.c)

apply the similar code to xtensa_hostfs.c.

i guess #15535 should be reverted until all backends are updated.

  • can you add a version for #ifndef CONFIG_HOST_LINUX which converts all errors to a generic one, eg. EIO?

OK, if you want.

ditto.

yamt added a commit to yamt/incubator-nuttx that referenced this pull request Apr 21, 2025
this has two purposes:

* reduce linux assumptions in sim
  (this is a follow-up of apache#15552)

* demonstrate errno.csv can be used for other purposes

Signed-off-by: YAMAMOTO Takashi <[email protected]>
yamt added a commit to yamt/incubator-nuttx that referenced this pull request Apr 21, 2025
this is a follow-up of apache#15552

Signed-off-by: YAMAMOTO Takashi <[email protected]>
yamt added a commit to yamt/incubator-nuttx that referenced this pull request Apr 21, 2025
this is a follow-up of apache#15552

Signed-off-by: YAMAMOTO Takashi <[email protected]>
yamt added a commit to yamt/incubator-nuttx that referenced this pull request Apr 21, 2025
tested with ELOOP, which is 62 on macOS and 40 on NuttX, on sim/macOS.

```shell
spacetanuki% ln -s a a
spacetanuki% cat a
cat: a: Too many levels of symbolic links
spacetanuki% ./nuttx

NuttShell (NSH) NuttX-10.4.0
nsh> mount -t hostfs -o fs=. /mnt
nsh> cat /mnt/a
nsh: cat: open failed: 40
nsh>
```

this is a follow-up of apache#15552
this fixes a non-linux regression in apache#15535

Signed-off-by: YAMAMOTO Takashi <[email protected]>
yamt added a commit to yamt/incubator-nuttx that referenced this pull request Apr 21, 2025
this has two purposes:

* reduce linux assumptions in sim
  (this is a follow-up of apache#15552)

* demonstrate errno.csv can be used for other purposes

Signed-off-by: YAMAMOTO Takashi <[email protected]>
yamt added a commit to yamt/incubator-nuttx that referenced this pull request Apr 21, 2025
this is a follow-up of apache#15552

Signed-off-by: YAMAMOTO Takashi <[email protected]>
yamt added a commit to yamt/incubator-nuttx that referenced this pull request Apr 21, 2025
this is a follow-up of apache#15552

Signed-off-by: YAMAMOTO Takashi <[email protected]>
yamt added a commit to yamt/incubator-nuttx that referenced this pull request Apr 21, 2025
tested with ELOOP, which is 62 on macOS and 40 on NuttX, on sim/macOS.

```shell
spacetanuki% ln -s a a
spacetanuki% cat a
cat: a: Too many levels of symbolic links
spacetanuki% ./nuttx

NuttShell (NSH) NuttX-10.4.0
nsh> mount -t hostfs -o fs=. /mnt
nsh> cat /mnt/a
nsh: cat: open failed: 40
nsh>
```

this is a follow-up of apache#15552
this fixes a non-linux regression in apache#15535

Signed-off-by: YAMAMOTO Takashi <[email protected]>
yamt added a commit to yamt/incubator-nuttx that referenced this pull request Apr 21, 2025
this has two purposes:

* reduce linux assumptions in sim
  (this is a follow-up of apache#15552)

* demonstrate errno.csv can be used for other purposes

Signed-off-by: YAMAMOTO Takashi <[email protected]>
yamt added a commit to yamt/incubator-nuttx that referenced this pull request Apr 21, 2025
this is a follow-up of apache#15552

Signed-off-by: YAMAMOTO Takashi <[email protected]>
yamt added a commit to yamt/incubator-nuttx that referenced this pull request Apr 21, 2025
this is a follow-up of apache#15552

Signed-off-by: YAMAMOTO Takashi <[email protected]>
yamt added a commit to yamt/incubator-nuttx that referenced this pull request Apr 21, 2025
tested with ELOOP, which is 62 on macOS and 40 on NuttX, on sim/macOS.

```shell
spacetanuki% ln -s a a
spacetanuki% cat a
cat: a: Too many levels of symbolic links
spacetanuki% ./nuttx

NuttShell (NSH) NuttX-10.4.0
nsh> mount -t hostfs -o fs=. /mnt
nsh> cat /mnt/a
nsh: cat: open failed: 40
nsh>
```

this is a follow-up of apache#15552
this fixes a non-linux regression in apache#15535

Signed-off-by: YAMAMOTO Takashi <[email protected]>
yamt added a commit to yamt/incubator-nuttx that referenced this pull request Apr 21, 2025
this has two purposes:

* reduce linux assumptions in sim
  (this is a follow-up of apache#15552)

* demonstrate errno.csv can be used for other purposes

Signed-off-by: YAMAMOTO Takashi <[email protected]>
yamt added a commit to yamt/incubator-nuttx that referenced this pull request Apr 21, 2025
this is a follow-up of apache#15552

Signed-off-by: YAMAMOTO Takashi <[email protected]>
yamt added a commit to yamt/incubator-nuttx that referenced this pull request Apr 21, 2025
this is a follow-up of apache#15552

Signed-off-by: YAMAMOTO Takashi <[email protected]>
yamt added a commit to yamt/incubator-nuttx that referenced this pull request Apr 21, 2025
tested with ELOOP, which is 62 on macOS and 40 on NuttX, on sim/macOS.

```shell
spacetanuki% ln -s a a
spacetanuki% cat a
cat: a: Too many levels of symbolic links
spacetanuki% ./nuttx

NuttShell (NSH) NuttX-10.4.0
nsh> mount -t hostfs -o fs=. /mnt
nsh> cat /mnt/a
nsh: cat: open failed: 40
nsh>
```

this is a follow-up of apache#15552
this fixes a non-linux regression in apache#15535

Signed-off-by: YAMAMOTO Takashi <[email protected]>
yamt added a commit to yamt/incubator-nuttx that referenced this pull request Apr 21, 2025
this has two purposes:

* reduce linux assumptions in sim
  (this is a follow-up of apache#15552)

* demonstrate errno.csv can be used for other purposes

Signed-off-by: YAMAMOTO Takashi <[email protected]>
yamt added a commit to yamt/incubator-nuttx that referenced this pull request Apr 21, 2025
this is a follow-up of apache#15552

Signed-off-by: YAMAMOTO Takashi <[email protected]>
yamt added a commit to yamt/incubator-nuttx that referenced this pull request Apr 21, 2025
this is a follow-up of apache#15552

Signed-off-by: YAMAMOTO Takashi <[email protected]>
yamt added a commit to yamt/incubator-nuttx that referenced this pull request Apr 21, 2025
tested with ELOOP, which is 62 on macOS and 40 on NuttX, on sim/macOS.

```shell
spacetanuki% ln -s a a
spacetanuki% cat a
cat: a: Too many levels of symbolic links
spacetanuki% ./nuttx

NuttShell (NSH) NuttX-10.4.0
nsh> mount -t hostfs -o fs=. /mnt
nsh> cat /mnt/a
nsh: cat: open failed: 40
nsh>
```

this is a follow-up of apache#15552
this fixes a non-linux regression in apache#15535

Signed-off-by: YAMAMOTO Takashi <[email protected]>
yamt added a commit to yamt/incubator-nuttx that referenced this pull request Apr 21, 2025
this has two purposes:

* reduce linux assumptions in sim
  (this is a follow-up of apache#15552)

* demonstrate errno.csv can be used for other purposes

Signed-off-by: YAMAMOTO Takashi <[email protected]>
yamt added a commit to yamt/incubator-nuttx that referenced this pull request Apr 21, 2025
this is a follow-up of apache#15552

Signed-off-by: YAMAMOTO Takashi <[email protected]>
yamt added a commit to yamt/incubator-nuttx that referenced this pull request Apr 21, 2025
this is a follow-up of apache#15552

Signed-off-by: YAMAMOTO Takashi <[email protected]>
yamt added a commit to yamt/incubator-nuttx that referenced this pull request Apr 21, 2025
tested with ELOOP, which is 62 on macOS and 40 on NuttX, on sim/macOS.

```shell
spacetanuki% ln -s a a
spacetanuki% cat a
cat: a: Too many levels of symbolic links
spacetanuki% ./nuttx

NuttShell (NSH) NuttX-10.4.0
nsh> mount -t hostfs -o fs=. /mnt
nsh> cat /mnt/a
nsh: cat: open failed: 40
nsh>
```

this is a follow-up of apache#15552
this fixes a non-linux regression in apache#15535

Signed-off-by: YAMAMOTO Takashi <[email protected]>
yamt added a commit to yamt/incubator-nuttx that referenced this pull request Apr 21, 2025
this has two purposes:

* reduce linux assumptions in sim
  (this is a follow-up of apache#15552)

* demonstrate errno.csv can be used for other purposes

Signed-off-by: YAMAMOTO Takashi <[email protected]>
yamt added a commit to yamt/incubator-nuttx that referenced this pull request Apr 21, 2025
this is a follow-up of apache#15552

Signed-off-by: YAMAMOTO Takashi <[email protected]>
yamt added a commit to yamt/incubator-nuttx that referenced this pull request Apr 21, 2025
this is a follow-up of apache#15552

Signed-off-by: YAMAMOTO Takashi <[email protected]>
yamt added a commit to yamt/incubator-nuttx that referenced this pull request Apr 21, 2025
tested with ELOOP, which is 62 on macOS and 40 on NuttX, on sim/macOS.

```shell
spacetanuki% ln -s a a
spacetanuki% cat a
cat: a: Too many levels of symbolic links
spacetanuki% ./nuttx

NuttShell (NSH) NuttX-10.4.0
nsh> mount -t hostfs -o fs=. /mnt
nsh> cat /mnt/a
nsh: cat: open failed: 40
nsh>
```

this is a follow-up of apache#15552
this fixes a non-linux regression in apache#15535

Signed-off-by: YAMAMOTO Takashi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: simulator Issues related to the SIMulator Size: S The size of the change in this PR is small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants