-
Notifications
You must be signed in to change notification settings - Fork 99
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
router: skip warning when configured lifetime is 0 #210
base: master
Are you sure you want to change the base?
Conversation
The configured lifetime is overridden to be 0 in certain cases where advertising a positive lifetime is not sensible. A log warning is only warranted if the user was not already aware of this condition and erroneously set the lifetime to a positive value. If the user acknowledges these conditions by manually configuring a 0 lifetime, we should not warn them of the override.
6ca8988
to
2b0472c
Compare
@@ -638,7 +638,7 @@ static int send_router_advert(struct interface *iface, const struct in6_addr *fr | |||
|
|||
if (default_route && valid_prefix) { | |||
adv.h.nd_ra_router_lifetime = htons(lifetime < UINT16_MAX ? lifetime : UINT16_MAX); | |||
} else { | |||
} else if (lifetime) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this change the logic flow here? The first if is only entered if both default_route && valid_prefix
evaluate to true. The second else
is always entered in all other cases. With this change, it is now only entered if lifetime
is true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes? That is the point of this patch. Can you clarify what is the issue?
I'm trying to remove this superfluous warning about overriding the ra_lifetime to 0. If the lifetime is already 0, there is no need to override it, and certainly no need to warn about overriding it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about the 'other' cases?
Before change: if a and b... else everything else end if
After change: if a and b... else if c... end if.
everything else is no longer handled. You might be right, I just haven't evaluated what everything else might be here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well the else block only does the override and the warning. If we don't want to do these things (because lifetime is already 0) I reckon there is nothing more to do.
The configured lifetime is overridden to be 0 in certain cases where advertising a positive lifetime is not sensible. A log warning is only warranted if the user was not already aware of this condition and erroneously set the lifetime to a positive value.
If the user acknowledges these conditions by manually configuring a 0 lifetime, we should not warn them of the override.