Skip to content

libc/unistd: move NAME_MAX/LINE_MAX/PATH_MAX define to unistd#15551

Merged
xiaoxiang781216 merged 2 commits into
apache:masterfrom
anchao:25011501
Jan 15, 2025
Merged

libc/unistd: move NAME_MAX/LINE_MAX/PATH_MAX define to unistd#15551
xiaoxiang781216 merged 2 commits into
apache:masterfrom
anchao:25011501

Conversation

@anchao

@anchao anchao commented Jan 15, 2025

Copy link
Copy Markdown
Contributor

Summary

  1. libc/unistd: move NAME_MAX/LINE_MAX/PATH_MAX define to unistd

These options are unistd-specific and should not be filesystem dependent,
and also not suitable for define in the sched directory.

Signed-off-by: chao an anchao.archer@bytedance.com

  1. libs/libc: rename LIBC_OPEN_MAX to OPEN_MAX

Signed-off-by: chao an anchao.archer@bytedance.com

  1. Documentation: clean up of CONFIG_NSH_LINELEN define

Signed-off-by: chao an anchao.archer@bytedance.com

Impact

N/A

Testing

ci-check

@github-actions github-actions Bot added Area: Documentation Improvements or additions to documentation Area: OS Components OS Components issues Board: arm Size: S The size of the change in this PR is small labels Jan 15, 2025
@nuttxpr

nuttxpr commented Jan 15, 2025

Copy link
Copy Markdown

[Experimental Bot, please feedback here]

This PR appears to mostly meet the requirements, but is missing some key information.

What's good:

  • Summary provides a decent overview of the what. It clearly lists the three changes being made.
  • Testing mentions ci-check, implying some automated testing. While not ideal, it's better than no testing information.

What's missing/needs improvement:

  • Summary lacks the why. It states what is being moved/renamed, but not why these changes are necessary. What problem do they solve? What's the benefit? For example, why are NAME_MAX, etc. "unistd-specific and should not be filesystem dependent"? What issues did this dependency cause? Similarly, why rename LIBC_OPEN_MAX? Why clean up CONFIG_NSH_LINELEN?
  • Impact is too dismissive. "N/A" is almost never true. Even seemingly small changes can have impacts. Consider:
    • Impact on user: Will any applications relying on the old names/locations break? Even if the answer is no, explain why.
    • Impact on build: Are there any conditional compilation changes? Again, even if no, explain why.
    • Impact on documentation: This is explicitly mentioned as changed, so the impact cannot be N/A. Describe what documentation was changed.
    • Impact on compatibility: Moving definitions can introduce subtle compatibility issues. Justify why there are none.
  • Testing is insufficient. ci-check tells us nothing specific. What targets were tested? What tests were run? What were the results of those tests? Providing "before" and "after" logs, even if they are identical, demonstrates due diligence. If the logs are voluminous, consider linking to a CI run instead of pasting them directly. At a minimum, specify the host OS, compiler, target architecture, and board configuration used for testing.

In short: The PR needs to expand on the why for each change and provide more concrete details regarding the impact and testing performed. Simply stating "N/A" or "ci-check" is not sufficient for a thorough review.

Comment thread libs/libc/unistd/Kconfig Outdated
Comment thread libs/libc/unistd/Kconfig
Comment thread boards/arm/nrf53/nrf5340-dk/configs/qspi_cpuapp/#defconfig# Outdated
@yamt

yamt commented Jan 15, 2025

Copy link
Copy Markdown
Contributor

and also not suitable for use in the sched directory.

why not?
they are kernel characteristics as far as i know.

@anchao

anchao commented Jan 15, 2025

Copy link
Copy Markdown
Contributor Author

why not? they are kernel characteristics as far as i know.

Let me clarify further, should be"define", not "use"

and also not suitable for define in the sched directory.

@yamt

yamt commented Jan 15, 2025

Copy link
Copy Markdown
Contributor

why not? they are kernel characteristics as far as i know.

Let me clarify further, should be"define", not "use"

and also not suitable for define in the sched directory.

well, to me, it seems more natural to define kernel constants in kernel than libc.

@anchao

anchao commented Jan 15, 2025

Copy link
Copy Markdown
Contributor Author

why not? they are kernel characteristics as far as i know.

Let me clarify further, should be"define", not "use"

and also not suitable for define in the sched directory.

well, to me, it seems more natural to define kernel constants in kernel than libc.

actually no one in the sched directory is using these definitions

@yamt

yamt commented Jan 15, 2025

Copy link
Copy Markdown
Contributor

why not? they are kernel characteristics as far as i know.

Let me clarify further, should be"define", not "use"

and also not suitable for define in the sched directory.

well, to me, it seems more natural to define kernel constants in kernel than libc.

actually no one in the sched directory is using these definitions

if you move them to fs, it might make sense.
but why libc?

@anchao

anchao commented Jan 15, 2025

Copy link
Copy Markdown
Contributor Author

why not? they are kernel characteristics as far as i know.

Let me clarify further, should be"define", not "use"

and also not suitable for define in the sched directory.

well, to me, it seems more natural to define kernel constants in kernel than libc.

actually no one in the sched directory is using these definitions

if you move them to fs, it might make sense. but why libc?

Most of the limit macros are Unix-specific and are more likely to be defined in libc unistd

https://pubs.opengroup.org/onlinepubs/009696799/functions/fpathconf.html

@yamt

yamt commented Jan 15, 2025

Copy link
Copy Markdown
Contributor

why not? they are kernel characteristics as far as i know.

Let me clarify further, should be"define", not "use"

and also not suitable for define in the sched directory.

well, to me, it seems more natural to define kernel constants in kernel than libc.

actually no one in the sched directory is using these definitions

if you move them to fs, it might make sense. but why libc?

Most of the limit macros are Unix-specific and are more likely to be defined in libc unistd

https://pubs.opengroup.org/onlinepubs/009696799/functions/fpathconf.html

sorry, i don't understand your logic.
i expect these constants (eg. PATH_MAX) are defined in a userland header (eg. limits.h) to match the kernel counterpart. (eg. CONFIG_PATH_MAX)
defining them in the opposite way sounds a bit awkward to me.

@anchao

anchao commented Jan 15, 2025

Copy link
Copy Markdown
Contributor Author

why not? they are kernel characteristics as far as i know.

Let me clarify further, should be"define", not "use"

and also not suitable for define in the sched directory.

well, to me, it seems more natural to define kernel constants in kernel than libc.

actually no one in the sched directory is using these definitions

if you move them to fs, it might make sense. but why libc?

Most of the limit macros are Unix-specific and are more likely to be defined in libc unistd
https://pubs.opengroup.org/onlinepubs/009696799/functions/fpathconf.html

sorry, i don't understand your logic. i expect these constants (eg. PATH_MAX) are defined in a userland header (eg. limits.h) to match the kernel counterpart. (eg. CONFIG_PATH_MAX) defining them in the opposite way sounds a bit awkward to me.

I just need a more suitable place to define these macros, not sched. unistd is the best choice I see. If you think there is a better place, please suggest instead of constantly saying no. What I need is the method, not your challenge.

Signed-off-by: chao an <anchao.archer@bytedance.com>
These options are unistd-specific and should not be filesystem dependent,
and also not suitable for define in the sched directory.

Signed-off-by: chao an <anchao.archer@bytedance.com>
@xiaoxiang781216 xiaoxiang781216 merged commit dedb57c into apache:master Jan 15, 2025
@yamt

yamt commented Jan 16, 2025

Copy link
Copy Markdown
Contributor

why not? they are kernel characteristics as far as i know.

Let me clarify further, should be"define", not "use"

and also not suitable for define in the sched directory.

well, to me, it seems more natural to define kernel constants in kernel than libc.

actually no one in the sched directory is using these definitions

if you move them to fs, it might make sense. but why libc?

Most of the limit macros are Unix-specific and are more likely to be defined in libc unistd
https://pubs.opengroup.org/onlinepubs/009696799/functions/fpathconf.html

sorry, i don't understand your logic. i expect these constants (eg. PATH_MAX) are defined in a userland header (eg. limits.h) to match the kernel counterpart. (eg. CONFIG_PATH_MAX) defining them in the opposite way sounds a bit awkward to me.

I just need a more suitable place to define these macros, not sched. unistd is the best choice I see. If you think there is a better place, please suggest instead of constantly saying no. What I need is the method, not your challenge.

i feel sched is a more natural place for them than libc.
if you are unhappy with sched, how about fs/Kconfig?

@anchao

anchao commented Jan 16, 2025

Copy link
Copy Markdown
Contributor Author

i feel sched is a more natural place for them than libc. if you are unhappy with sched, how about fs/Kconfig?

Sure, except for the sched directory, libc/fs is more reasonable. I think the reason why libc is better is that the limit value is frequently used in user space, so libc may be better.

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

Labels

Area: Documentation Improvements or additions to documentation Area: OS Components OS Components issues 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.

5 participants