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.

Sign up for GitHub

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

Jump to bottom

*: clean up zclient options #14867

Merged
merged 8 commits into from
Nov 25, 2023
Merged

*: clean up zclient options #14867

merged 8 commits into from
Nov 25, 2023

Conversation

eqvinox
Copy link
Contributor

@eqvinox eqvinox commented Nov 23, 2023

  • remove receive_notify, use ZEBRA_ROUTE_NOTIFY_REQUEST instead
  • add a common predefined zclient_options_sync
  • make the options const
  • add an auxiliary option that prevents lib/ message handlers from being called
    • note this only affects VNC since that was the only place that opens a second zclient that is NOT also synchronous

@eqvinox
pbrd: replace receive_notify with request
500a09d 
Send `ZEBRA_ROUTE_NOTIFY_REQUEST` rather than relying on the options
field in zclient startup.

Signed-off-by: David Lamparter <[email protected]>
@eqvinox
sharpd: replace receive_notify with request
adf7b9e 
Send `ZEBRA_ROUTE_NOTIFY_REQUEST` rather than relying on the options
field in zclient startup.

Signed-off-by: David Lamparter <[email protected]>
@eqvinox
staticd: replace receive_notify with request
f1a15bd 
Send `ZEBRA_ROUTE_NOTIFY_REQUEST` rather than relying on the options
field in zclient startup.

Signed-off-by: David Lamparter <[email protected]>
@eqvinox
eigrpd: use default zclient options
7f7564b 
`.receive_notify = false` is the default.

Signed-off-by: David Lamparter <[email protected]>
@eqvinox
lib: remove .receive_notify zclient option
25a1dcc 
This should just be set with `ZEBRA_ROUTE_NOTIFY_REQUEST` instead.

Signed-off-by: David Lamparter <[email protected]>
@eqvinox
zebra, lib: remove notify field from hello message
a13d293 
This is no longer used.

Signed-off-by: David Lamparter <[email protected]>
@eqvinox
*: add zclient_options_sync
cc90c54 
... and use it instead of fiddling with the `.synchronous` field.

(Make it const while at it.)

Signed-off-by: David Lamparter <[email protected]>
@eqvinox
lib, bgp/vnc: add .auxiliary zclient option
5a40f2b 
Avoids calling VRF/interface/... handlers in library code more than
once.  It's kinda surprising that this hasn't been blowing up already
for the VNC code, luckily these handlers are (mostly?) idempotent.

Signed-off-by: David Lamparter <[email protected]>
donaldsharp
donaldsharp reviewed Nov 23, 2023
View reviewed changes
lib/zclient.c Outdated
@@ -41,8 +41,9 @@ static void zclient_event(enum zclient_event, struct zclient *);
static void zebra_interface_if_set_value(struct stream *s,
struct interface *ifp);

struct zclient_options zclient_options_default = {.receive_notify = false,
.synchronous = false};
struct zclient_options zclient_options_default = {
Copy link
Member

Choose a reason for hiding this comment

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

If I am testing via bisect, won't this change cause and assert and crash in zebra every time? This makes me unhappy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

???

Why would this crash anything?

Copy link
Member

Choose a reason for hiding this comment

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

In this commit the byte of data sent in the send_hello function is removed. The equivalent code in zebra to read this will now crash as that it will try to read 1 byte beyond the end of the stream, as that we have code that intentionally crashes when this happens. As I stated if this commit is choosen as the bisect I will be stopped in my tracks if I attempt to run zebra with any other daemon at all to find a different problem. This is especially egregious from my perspective because I cannot work around this issue at all as that the initial connection from any daemon will cause zebra to crash

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The byte is not removed in this commit. Please flush your brain cache and look at the commit again 😆
25a1dcc#diff-f949c6f0ae3101476f0699de68a4a73935fdd945294d5f235e97fb5e8bdbc782L395-R395

donaldsharp
donaldsharp reviewed Nov 23, 2023
View reviewed changes
lib/zclient.c Outdated
@@ -392,7 +392,6 @@ enum zclient_send_status zclient_send_hello(struct zclient *zclient)
stream_putc(s, zclient->redist_default);
stream_putw(s, zclient->instance);
stream_putl(s, zclient->session_id);
stream_putc(s, 0); /* receive_notify - removed */
Copy link
Member

Choose a reason for hiding this comment

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

I would combine this commit with the previous one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've intentionally separated it since it changes the ZAPI protocol & we know there are at least some external users (e.g. gobgp)

donaldsharp
donaldsharp reviewed Nov 23, 2023
View reviewed changes
bgpd/rfapi/vnc_zebra.c
@@ -872,7 +872,7 @@ static zclient_handler *const vnc_handlers[] = {
void vnc_zebra_init(struct event_loop *master)
{
/* Set default values. */
zclient_vnc = zclient_new(master, &zclient_options_default,
zclient_vnc = zclient_new(master, &zclient_options_auxiliary,
Copy link
Member

Choose a reason for hiding this comment

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

Where is the zapi communication of this change into zebra and it respecting this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why would this have any effect on zebra? The goal is to not invoke the common lib/ callbacks on the receiving end in the daemon. There can still be other code that wants to process the same notifications (e.g. running multithreaded), just without duplicating/going into lib/ structures.

donaldsharp
donaldsharp requested changes Nov 23, 2023
View reviewed changes
Copy link
Member

@donaldsharp donaldsharp left a comment

Choose a reason for hiding this comment

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

couple ofthings need to be cleaned up

@eqvinox eqvinox requested a review from donaldsharp November 24, 2023 12:56
@donaldsharp donaldsharp merged commit 0dc7704 into FRRouting:master Nov 25, 2023
83 checks passed
@eqvinox eqvinox deleted the zclient-options-cleanup branch November 27, 2023 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@donaldsharp donaldsharp Awaiting requested review from donaldsharp

Assignees
No one assigned
Labels
eigrp libfrr master pbr sharp size/L staticd zebra
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@eqvinox @donaldsharp