Skip to content
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.

Sign up for GitHub

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

Jump to bottom

libc: modify open,close... to _NX_OPEN,_NX_CLOSE #14651

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

libc: modify open,close... to _NX_OPEN,_NX_CLOSE #14651

wants to merge 1 commit into from
+507 −267

Conversation

W-M-R
Copy link
Contributor

@W-M-R W-M-R commented Nov 5, 2024

Summary

libc: modify open,close... to _NX_OPEN,_NX_CLOSE

Impact

None

Testing

None

@github-actions github-actions bot added Area: OS Components OS Components issues Size: L The size of the change in this PR is large labels Nov 5, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 5, 2024

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements. While it provides a summary of the changes, it lacks crucial details. Specifically:

  • Insufficient Summary: "modify open,close... to _NX_OPEN,_NX_CLOSE" is vague. Why was this change necessary? What problem does it solve? What is the functional impact? How does using _NX_ prefixed functions change the behavior?
  • Incomplete Impact Assessment: Claiming "None" for impact is highly unlikely. At minimum, this change affects compatibility. Does it break existing code? Does it improve portability? The impact assessment needs to be thoroughly considered and documented.
  • Missing Testing Information: "None" for testing is unacceptable. Every PR requires testing. What platforms was this tested on (host and target)? What tests were run? What were the results? Providing "before" and "after" logs, even if they appear identical, demonstrates that testing was performed.

This PR needs significant revision before it can be considered. The author needs to provide specific details in each section to demonstrate that the change is well-motivated, thoroughly tested, and doesn't introduce unintended consequences.

@xiaoxiang781216
Copy link
Contributor

@W-M-R Please fix:

Error: /home/runner/work/nuttx/nuttx/nuttx/libs/libc/lzf/lzf_d.c:5:80: error: Long line found

@acassis
Copy link
Contributor

acassis commented Nov 7, 2024

@W-M-R I don't remember why these changes were made. Is it necessary to separate kernel from user space APIs?

pkarashchenko
pkarashchenko reviewed Nov 8, 2024
View reviewed changes
libs/libc/dirent/lib_fdopendir.c Show resolved Hide resolved
libs/libc/dirent/lib_nftw.c
@@ -198,7 +200,7 @@ do_nftw(FAR char *path, nftw_cb_t fn, int fdlimit, int flags, int level)

if (strlen(de->d_name) > PATH_MAX - j)
{
set_errno(ENAMETOOLONG);
_NX_SETERRNO(ENAMETOOLONG);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems like we need a similar change at line 113 and 164: get_errno -> _NX_GETERRNO

@W-M-R W-M-R closed this Nov 11, 2024
@W-M-R W-M-R deleted the modify_file_name branch November 11, 2024 03:48
@W-M-R
Copy link
Contributor Author

W-M-R commented Nov 11, 2024

#14651

@W-M-R I don't remember why these changes were made. Is it necessary to separate kernel from user space APIs?

Yes, kernel mode and user mode need to use different APIs

@W-M-R W-M-R restored the modify_file_name branch November 11, 2024 04:04
@W-M-R
Copy link
Contributor Author

W-M-R commented Nov 11, 2024

Sorry, I accidentally disabled this patch.

@W-M-R W-M-R reopened this Nov 11, 2024
@W-M-R
libc: modify open,close... to _NX_OPEN,_NX_CLOSE
4c560a1 
Signed-off-by: wangmingrong1 <wangmingrong1@xiaomi.com>
@W-M-R
Copy link
Contributor Author

W-M-R commented Nov 11, 2024

The automatic test failed. Please give me some time to check it carefully.

@xiaoxiang781216
Copy link
Contributor

need rebase to fix the conflict

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pkarashchenko pkarashchenko pkarashchenko left review comments

@acassis acassis Awaiting requested review from acassis

Assignees
No one assigned
Labels
Area: OS Components OS Components issues Size: L The size of the change in this PR is large
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@W-M-R @nuttxpr @xiaoxiang781216 @acassis @pkarashchenko