Skip to content

Improve babel_send() error handling#109

Open
DanielG wants to merge 1 commit into
jech:masterfrom
DanielG:master
Open

Improve babel_send() error handling#109
DanielG wants to merge 1 commit into
jech:masterfrom
DanielG:master

Conversation

@DanielG

@DanielG DanielG commented Jul 19, 2023

Copy link
Copy Markdown
Contributor

Fixes for #108

This is untested, I'm not sure we need to print scope_id but the thinking is this would tell you which interface is involved.

Printing the wireguard error only once ever is the lazy apporach, I suppose it should be per-interface or something really? @jech opinion?

@DanielG

DanielG commented Jul 19, 2023

Copy link
Copy Markdown
Contributor Author

Looks like the scope_id is set properly but a final newline was still missing in the fprintf(). Should be good now.

@jech

jech commented Jul 20, 2023

Copy link
Copy Markdown
Owner

Concerning the first patch, please use format_address or format_prefix in util.c.

Concerning the second patch, I think that printing the error just once is too drastic. Instead, you should rate-limit the error, for example by printing it at most once every 30 seconds.

- Print EDESTADDREQ only once per interface down/up cycle
- Include destination address in error message
@DanielG

DanielG commented Jul 21, 2023

Copy link
Copy Markdown
Contributor Author

Since the error really is just noise I really don't want to print it periodically, even at a low rate. Instead I make this a per-interface flag. Note I'm not sure I clear the flag in the right place, the idea is to reset whenever interface comes back up from being down.

Let me know if you're ok with this version else I'll go with special handling wireguard interfaces.

@jech

jech commented Aug 6, 2023

Copy link
Copy Markdown
Owner

Since the error really is just noise I really don't want to print it periodically, even at a low rate.

I disagree. Suppressing the error after the first occurrence will make debugging more difficult.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants