-
Notifications
You must be signed in to change notification settings - Fork 30
add missing headers for some system functions #237
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
Conversation
mbhangui
left a comment
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.
LGTM
|
Is there a particular compiler warning level you're using to make these pop out? Something we could include in one of the autobuilds? |
schmonz
left a comment
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.
In a different situation I might want to block this on having our autobuilds auto-detect popular merge conflicts. Right now it's more important to me that we start moving again. Thank you for the impetus.
| #include <sys/socket.h> | ||
| #include <netinet/in.h> | ||
| #include <arpa/inet.h> | ||
| #include <unistd.h> |
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.
In general this change is of course correct and needed, and as usual I'm mildly worried only about the timing of the change in programs which many users are likely to still be heavily patching: this one and smtpd, mainly.
@schmonz gonna schmonz ;-) (and seriously, it's nice to be back here thinking about notqmail, so thank you) -- but I would love to be able to feel that this sort of thing is getting worried about automatically. Then I could find new kinds of things to be mildly annoying about. @josuah, how can we help finish getting the patch-merge-checker into our regular autobuilds?
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.
As this a bit older: no, I don't think so. At least I can't provoke a warning on my system, most likely because (at least today, maybe not when these patches were originally done) these headers are in one or another indirect way automatically included. So for now I would call it "manual code audit", even if it was not true back then it would be true if I had to do them again.
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.
This has been split out of #65. The commit about headers for
umask()has been omitted asmode_tis not used, so<sys/types.h>isn't actually needed anywhere for this.