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

Reapply "SYSLOG_DEFAULT: wrap up_putc/up_nputs calls with critical section" with a fix #14761

Merged
merged 2 commits into from
Nov 18, 2024

Conversation

yamt
Copy link
Contributor

@yamt yamt commented Nov 13, 2024

Summary

Reapply "SYSLOG_DEFAULT: wrap up_putc/up_nputs calls with critical section" with a fix.

original change #14722
a revert #14751

Impact

Testing

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

nuttxpr commented Nov 13, 2024

[Experimental Bot, please feedback here]

No. The PR is missing critical information required by the NuttX template.

  • Summary: While it mentions the original PR and revert, it lacks a clear explanation of why the reapplication is necessary and how the fix addresses the issues that led to the revert. What exactly was the fix?
  • Impact: The entire Impact section is missing. All fields should be addressed with either "NO" or a "YES" + explanation. This is crucial for reviewers to understand the potential consequences of the change.
  • Testing: While targets are listed, the "Testing logs before change" and "Testing logs after change" sections are empty. This makes it impossible to verify the effectiveness of the fix. What were the symptoms of the bug before, and how do the logs demonstrate that the bug is now resolved? Furthermore, mentioning "a few other unrelated local patches" raises concerns; testing should ideally be done on a clean branch to isolate the effects of the PR.

Therefore, the PR in its current state does not meet the NuttX requirements. It needs substantial additions to be considered complete.

sched/irq/irq_csection.c Outdated Show resolved Hide resolved
Copy link
Contributor

@pussuw pussuw left a comment

Choose a reason for hiding this comment

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

LGTM. This also fixes recursive panic/assertion for me when trying to take the spinlock from assert!

sched/irq/irq_csection.c Outdated Show resolved Hide resolved
@xiaoxiang781216
Copy link
Contributor

@yamt please fix the following warning:

Error: syslog/syslog_channel.c:231:13: error: 'csection_available' defined but not used [-Werror=unused-function]
 static bool csection_available(void)
             ^

Note that SYSLOG_DEFAULT can be used in extreme situations,
including very early in the boot.
@xiaoxiang781216 xiaoxiang781216 merged commit 637a0f5 into apache:master Nov 18, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Drivers Drivers 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.

7 participants