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

Handle unset PID #57

Merged
merged 2 commits into from
Oct 27, 2019
Merged

Handle unset PID #57

merged 2 commits into from
Oct 27, 2019

Conversation

vorner
Copy link
Contributor

@vorner vorner commented Oct 18, 2019

No description provided.

@vorner
Handle unset PID
f182835 
Some protocols (eg. NFLOG) don't set the PID and leave it to 0 all the
time on some kinds of messages. Don't get angry at them with errors,
this is fine and just ignore the PID checks when it is 0.

Closes jbaublitz#46.
@vorner vorner mentioned this pull request Oct 18, 2019
Merged
@jbaublitz
Copy link
Owner

I'm looking at this now, but I'm actually considering making None indicate that PID checking is disabled.

From what I understand a PID of 0 indicates that the message is being sent to the kernel. Therefore, if it's None, ignore the PID checking. If it's Some(0), overwrite it with the PID assigned to the socket when the user binds the socket. This seems like a friendlier way of navigating this problem as None really shouldn't be used both as an initialization value and also an indication that PID checking should be used. That's overloading the semantic meaning of the variant.

What do you think?

@vorner
Copy link
Contributor Author

vorner commented Oct 24, 2019

Well, the problems I had were with messages coming from the kernel, which had PID 0. But otherwise I have no idea what the purpose of the PID field or the actual check of it is.

I mean, in which case would a wrong message get delivered and the library would be supposed to fail on it?

Anyway, I'm OK with changing it that way. I'll send an updated code some time soon.

@vorner
Copy link
Contributor Author

vorner commented Oct 26, 2019

Added as a fixup. I can either rewrite the branch or you probably can squash the whole branch to a single commit at merge (as it is effectively only one commit).

@jbaublitz jbaublitz self-requested a review October 27, 2019 15:14
@jbaublitz
Copy link
Owner

This is great, thanks!

@jbaublitz jbaublitz merged commit b9f930f into jbaublitz:master Oct 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jbaublitz jbaublitz Awaiting requested review from jbaublitz

Assignees

@vorner vorner

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@vorner @jbaublitz