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

send, test/send: isolate cease_adv of add_ra_header #201

Merged
merged 4 commits into from
Jan 14, 2023

Conversation

robbat2
Copy link
Member

@robbat2 robbat2 commented Jan 9, 2023

The add_ra_header function had a hidden dependency on system state:
If cease_adv was not set, the outcome was dependent on the sysctl knob
"net.ipv6.conf.all.forwarding".

This was visible only during testing with
net.ipv6.conf.all.forwarding=1. If tested with
net.ipv6.conf.all.forwarding=0, the tests passed.

Hoist the sysctl dependency out of the add_ra_header function, to enable
testing of both code paths, and add the extra test to validate both
states of the cease_adv input.

Also improve snprint_safe_buffer:

Closes: #200
Signed-off-by: Robin H. Johnson [email protected]

The add_ra_header function had a hidden dependency on system state:
If cease_adv was not set, the outcome was dependent on the sysctl knob
"net.ipv6.conf.all.forwarding".

This was visible only during testing with
net.ipv6.conf.all.forwarding=1. If tested with
net.ipv6.conf.all.forwarding=0, the tests passed.

Hoist the sysctl dependency out of the add_ra_header function, to enable
testing of both code paths, and add the extra test to validate both
states of the cease_adv input.

Closes: radvd-project#200
Signed-off-by: Robin H. Johnson <[email protected]>
@robbat2 robbat2 force-pushed the fix-cease-adv-testcase branch from c2dc8c9 to 9db91d5 Compare January 9, 2023 17:12
Copy link
Contributor

@mpontillo mpontillo left a comment

Choose a reason for hiding this comment

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

Looks good in general; I have a couple minor comments.

test/print_safe_buffer.c Show resolved Hide resolved
test/send.c Show resolved Hide resolved
test/send.c Outdated Show resolved Hide resolved
Signed-off-by: Robin H. Johnson <[email protected]>
The existing count-- placement inside the loop removed the commas after
each byte. Keep the commas.

Signed-off-by: Robin H. Johnson <[email protected]>
@robbat2 robbat2 requested a review from mpontillo January 14, 2023 20:37
Copy link
Contributor

@mpontillo mpontillo left a comment

Choose a reason for hiding this comment

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

lgtm! 🚢

@robbat2 robbat2 merged commit 17f1828 into radvd-project:master Jan 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Build failure with test_add_ra_header on aarch64
2 participants