[Toybox] [Patch] - new toy "dhcp6", patches for dhcp and syslogd to support IPv6.

[Rob Landley]

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

 1446601867.0