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

Improve error handling in p0f plugin #175

Merged
merged 2 commits into from
Dec 30, 2014
Merged

Improve error handling in p0f plugin #175

merged 2 commits into from
Dec 30, 2014

Conversation

flimzy
Copy link
Contributor

@flimzy flimzy commented Dec 30, 2014

  • This corrects a specific bug, where a socket connection failure was never reported (due to reporting $@ instead of $!)
  • And it makes all the present error logs more meaningful by mentioning 'p0f'

Jonathan Hall added 2 commits December 30, 2014 06:23
- IO::Socket::UNIX->new should never die, therefore...
- $@ is always undef when the eval exits, so...
- use $! instead
- Add "p0f" to error output to aide in debugging
- Remove some no-ops which called only '... if $@', in a context
  where $@ would never be set, or never be meaningful.
jaredj added a commit that referenced this pull request Dec 30, 2014
Improve error handling in p0f plugin
@jaredj jaredj merged commit 562fa95 into smtpd:master Dec 30, 2014
@flimzy flimzy deleted the p0f-errors branch December 30, 2014 14:56
@msimerson
Copy link
Member

And it makes all the present error logs more meaningful by mentioning 'p0f'

Does this not just make all them error messages more redundant? Last I checked, plugin log messages look like this:

@4000000054a2ff8d077dfddc 98226 (connect) ident::p0f: FreeBSD 9.x or newer

So before you change, an example error message was:

@4000000054a2ff8d077dfddc 98226 (connect) ident::p0f: skip, unable to determine remote IP

and now it's:

@4000000054a2ff8d077dfddc 98226 (connect) ident::p0f: skip p0f, unable to determine remote IP

@jaredj
Copy link
Contributor

jaredj commented Dec 30, 2014

@msimerson what is responsible for prepending '(connect) ident::p0f: '? perhaps it's something that doesn't exist in our fork yet... or doesn't exist in upstream yet? :)

@msimerson
Copy link
Member

@jaredj, I dunno. QP has done that for as long as I've used it.

@msimerson
Copy link
Member

what is responsible for prepending (connect) ident::p0f:?

lib/Qpsmtpd::varlog

@flimzy
Copy link
Contributor Author

flimzy commented Dec 31, 2014

I suspect the code to prepend the plugin name, etc, is missing or non-functional in our fork, then. I'm in favor of rolling back that particular commit here, and investigating further on our fork.

@jaredj
Copy link
Contributor

jaredj commented Dec 31, 2014

Not exactly; see issue #182

At any rate this PR has already been un-merged

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.

3 participants