On 10/20/2015 01:30 AM, Sameer Pradhan wrote: > Hi Rob & List, > > Attached are two patches and a new toy. > dhcp6.c: New toy "dhcp6", the IPv6 client for dynamic network configuration. > > dhcp.c: Attaching the patch for "dhcp.c" to support the "dhcp6.c". > > syslogd.c : Patch for syslogd.c to support IPv6.
Sorry, I've had this window open on my desktop for weeks, meaning to get to it but trying to get a release out first. A lot of the changes are good cleanups, and adding a new dhcp6.c file is fine and doesn't require much attention for the first pass, but if dhcp.c is going to shell out to dhcp6 (the fact you're patching it implies this is the case), I'd prefer to have them in the same *.c file and let one call the other as a C function rather than an unnecessary exec() and possible $PATH search. In http://landley.net/toybox/code.html#lib_args it says: Put globals not filled out by the option parsing logic at the end of the GLOBALS block. Common practice is to list the options one per line (to make the ordering explicit, first to last in globals corresponds to right to left in the option string), then leave a blank line before any non-option globals. A lot of your patches collate the option argument variables on the same line, when they were on different lines for a reason. You also mix a lot of whitespace changes with functional changes, which not only makes it hard for me to figure out if your dhcp.c.patch is adding a shell-out to your new dhcp6 command, but it's something I try to avoid in general. When you re-indent a lot of stuff with the occasional code change, it makes it hard to spot the code changes. (You can re-diff with -b after the fact, but it makes it hard to read the patch before applying it, or for tools like "git annotate" to see through. If you annotate to a whitespace-only patch you can re-annotate on COMMIT^1 without too much trouble, but if it's whitespace _and_ functional changes you can't ignore it as easily and it becomes persistent gotcha for future annotates...) Adding xwrite() to dhcp: if you have a persistent server process, you either don't want to use xfuncs() (which calls xexit() on error) or you need to set up a toys.rebound longjmp handler to intercept and resume. So I'm all for error handling, but _what_ error handling is the question. Has to be context-appropriate. Stuff in pending should not 'default y'. Not only does the new command default y, but you change existing pending commands to default y. Please don't add typedefs, just say "struct structname" or the actual type. I've added the dhcp6.c to pending (default n). The rest I need to take another look at later, maybe try to chip out the changes I want to keep from the unrelated stuff. Thanks, Rob _______________________________________________ Toybox mailing list [email protected] http://lists.landley.net/listinfo.cgi/toybox-landley.net
