Skip to content

fix ICMPv6 NEIGHBOR ADVERTISE#17371

Open
snikeguo wants to merge 1 commit intoapache:masterfrom
snikeguo:master
Open

fix ICMPv6 NEIGHBOR ADVERTISE#17371
snikeguo wants to merge 1 commit intoapache:masterfrom
snikeguo:master

Conversation

@snikeguo
Copy link

@snikeguo snikeguo commented Nov 24, 2025

Summary

Update this section with information on why change is necessary,
what it exactly does and how, if new feature shows up, provide
references (dependencies, similar problems and solutions), etc.

Impact

Update this section, where applicable, on how change affects users,
build process, hardware, documentation, security, compatibility, etc.

Testing

This section should provide a detailed description of what you did
to verify your changes work and do not break existing code.

Please provide information about your host machine, the board(s) you
tested your changes on, and how you tested. Logs should be included.

For example, when changing something in the core OS functions, you
may want to run the OSTest application to verify that there are no
regressions. Changes to ADC code may warrant running the adc
example. Adding a new uORB driver may require that you run
uorb_listener to verify correct operation.

Pure documentation changes can just be tested with make html
(see docs) and verification of the correct format in your
browser.

PRs without testing information will not be accepted. We will
request test logs.

@github-actions github-actions bot added Area: Networking Effects networking subsystem Size: S The size of the change in this PR is small labels Nov 24, 2025
@simbit18
Copy link
Contributor

simbit18 commented Nov 24, 2025

Hi @snikeguo, please read the guidelines for contributions

https://github.com/apache/nuttx/blob/master/CONTRIBUTING.md#15-commit-requirements

../nuttx/tools/checkpatch.sh -c -u -m -g df3fe0e16f87736a8f4fd31909b5fe1bcf2a91e7..HEAD
❌ Commit subject missing colon (e.g. 'subsystem: msg')
❌ Missing Signed-off-by

A summary is also required for the PR

https://github.com/apache/nuttx/blob/master/.github/PULL_REQUEST_TEMPLATE.md

{
FAR struct icmpv6_neighbor_advertise_s *adv;

bool should_process = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: use a more explicit name, it helps people that will read this code in the future. I suggest using "is_for_this_node" instead of "should_process". Although in the "else" it tests for allnodes the packet needs to be handle locally. Other option could be "is_local_or_allnodes_dest"

{
should_process = true;
}
else if (net_ipv6addr_cmp(ipv6->destipaddr, g_ipv6_allnodes))
Copy link
Contributor

Choose a reason for hiding this comment

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

why not merge into one if statement

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this idea was to detect if received some NA and print display it

Copy link
Contributor

@linguini1 linguini1 left a comment

Choose a reason for hiding this comment

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

Please fill out the PR according to the template.

Copy link
Contributor

@cederom cederom left a comment

Choose a reason for hiding this comment

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

Thank you @snikeguo, please read carefully the https://github.com/apache/nuttx/blob/master/CONTRIBUTING.md, then update PR and git commits :-)

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

Labels

Area: Networking Effects networking subsystem 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.

6 participants