Re: Making DHCP renew work in pfinet

[Stefan Siegl]

Hello Christian,

On Sun, Oct 14, 2007 at 03:36:06PM +0200, Christian Dietrich wrote:
> hurd source package. In order to make the routing for the dhcp renew
> work. I do always add an route for 0.0.0.0 and the dhcp ports to the
> devices. 

well, I haven't yet tested every change that your patch introduces
(mainly using multiple ethernet interfaces), but some thoughts that come
to my mind:

> The whitespaces beetween -a and the ip are missing because -a, -g, -m,
> -p, -A, -G have only optional arguments. If the argument is no passed
> the value will be unset e.g. the gateway.

I'm afraid, but I don't think that's a good idea.  People are used to
passing `-a 192.168.7.5' or similar and the provided IP addresses can be
distinguished quite easily.

If a user provides the afore mentioned option, parse_opt is called with
key == `a' and arg == NULL.  However ``state->argv[state->next]''
provides a pointer to the IP address which should be examined in turn.
Unless you consider the bit another option, i.e. you cannot consume it
as some IP address, simply increment state->next.

>  if [ x$reason = xPREINIT ]; then
> -  settrans -afg /servers/socket/2 /hurd/pfinet --dhcp -i $interface
>    exit_with_hooks 0
>  fi

I'm unsure concerning this, but I think you should check whether there
is some pfinet installed and set it in case not.

> +  {"dhcp",   'd', 0        , 0, "Prepare pfinet for dhcp"},

I don't think you should introduce the `--dhcp' option after all.  It
isn't quite needed anyways, since you unconditionally set up the routing
needed for dhcp-client.  The option name itself is controversial (since
misleading, pfinet doesn't do dhcp discovery by iteself). Maybe see
discussions of Marco and Roland, that have taken place in April 2005
mainly.

Users (i.e. dhcp-client script) can simply pass `-a 0.0.0.0', if there
is no known address available.

> +static void
> +parse_interface_copy_device(struct device *src,
> +                            struct parse_interface *dst)
> +{
> [...]
> +    /* Look for IPv6 default route (we use the first ifa->addr as
> +       source address), but don't yet push it to the option stack. */

This comment doesn't fit here, at least not the latter part of the
sentence.

Furthermore there is a function `ipv6_get_dflt_router' now, that should
just does the job you need.

> -         /* Delete any existing default route.  */
> +         /* Delete any existing dflt route on configured devices  */

Please don't do this, there's no need for using such abbreviations in
comments (in function names maybe), it just makes them less readable.
Another line of source code should not hurt.

Omit using deprecated functions, `bzero' namely.  Replace these calls by
those to `memset'.

Last but not least I'd like to ask you to review your patch regarding
whitespace changes.  There are quite a few hunks in it, resulting from
those.  Especially there were no changes to timer-emul.c at all, apart
from whitespace ones.


Thanks for your work, you know that lease-renew'ing dhcp-support has
often been asked for.  And of course welcome on board :-)

cheers,
  stesie