Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
usr.bin/elfdump/elfdump.c | ||
---|---|---|
493 | My understanding is that we sort in descending order of sizeof(), then ascending alphabetical order. I also couldn't find a hard-and-fast rule in style(9), but I think this is the generally-accepted principle. Given my relationship with style(9), however, you should take this comment with a rather large grain of salt. | |
552 | It's a good idea to: you wouldn't want a malicious ELF binary to gain control of your terminal session (e.g., be able to send funny ioctls) when you inspect it. Such highjacking should be reserved for when you *run* a malicious binary. :) |
Overall looks good to me. I do agree with Jonathan that we should also limit std descriptors. You can find example in the go_daemon() function in sbin/dhclient/dhclient.c.
usr.bin/elfdump/elfdump.c | ||
---|---|---|
31–32 | You should always include sys/types.h first. It is good to sort kernel headers, but it is not always possible (it should always be possible for userland headers, though). sys/types.h and sys/param.h are exceptions and should always be included first (but only one of them). |
Just a couple more inline comments. Other than these, and with the caveat that I haven't run/tested the proposed patch, I think this looks good.
usr.bin/elfdump/elfdump.c | ||
---|---|---|
32 | This is an extremely picky point, but should we keep the newline between sys/types.h and the other sys/ headers? That is: #include <sys/types.h> #include <sys/capsicum.h> #include <sys/elf32.h> /* ... */ | |
553 | Maybe STDIN_FILENO as well? Or even better, maybe close(STDIN_FILENO) ? |