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.

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

modlib/meminfo: trivial revision on logging/dump #15640

Merged
merged 2 commits into from
Jan 22, 2025

Conversation

yf13
Copy link
Contributor

@yf13 yf13 commented Jan 22, 2025

Summary

Cosmetic revisions on modlib logging and meminfo dump format.

Impacts

None

Testing

  • local check with qemu-armv7a:knsh
  • CI checks

@github-actions github-actions bot added Area: File System File System issues Area: OS Components OS Components issues Size: S The size of the change in this PR is small labels Jan 22, 2025
@nuttxpr
Copy link

nuttxpr commented Jan 22, 2025

[Experimental Bot, please feedback here]

Yes, this PR appears to meet the NuttX requirements, although minimally. While technically compliant, it would benefit from more detail, especially given the vagueness of "cosmetic revisions." Specifically:

  • Summary: While it states what changed, it doesn't explain why. Even for cosmetic changes, a brief rationale (e.g., "improved readability," "consistent formatting with other modules") is helpful. It should also specify which modules in modlib were affected.
  • Impacts: Stating "None" is acceptable if truly no impact, but this needs careful consideration. Did the log output change in any way that could affect scripts parsing those logs? Even minor formatting changes could have unintended consequences.
  • Testing: "local check" is insufficient. Provide the actual commands used (e.g., make distclean && configure sim:qemu-armv7a:knsh && make && make run), and ideally, snippets of the before/after log output demonstrating the cosmetic improvements. CI passing is good, but doesn't replace showcasing the specific changes this PR introduces.

In short, while technically meeting the letter of the requirements, the PR would be stronger with more specific information to demonstrate due diligence and facilitate review.

libs/libc/modlib/modlib_insert.c Outdated Show resolved Hide resolved
libs/libc/modlib/modlib_insert.c Outdated Show resolved Hide resolved
libs/libc/modlib/modlib_insert.c Outdated Show resolved Hide resolved
yf13 added 2 commits January 22, 2025 17:47
This guards `.modname` field usage with same condition as header
definitions.

Signed-off-by: Yanfeng Liu <[email protected]>
meminfo before:
```
      total       used       free    maxused    maxfree  nused  nfree name
    3129344      10824    3118520      11184    3118104     25      2 Kmem
   13631488    1114112   12517376   12517376 Page
```
and after:

```
      total       used       free    maxused    maxfree  nused  nfree name
    3129344      10824    3118520      11184    3118104     25      2 Kmem
   13631488    1114112   12517376              12517376               Page
```
Signed-off-by: Yanfeng Liu <[email protected]>
@xiaoxiang781216 xiaoxiang781216 merged commit 4e8bcf1 into apache:master Jan 22, 2025
39 checks passed
@yf13 yf13 deleted the modlib branch January 30, 2025 07:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: File System File System issues 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.

4 participants