Peter Memishian writes:
> Sorry for the delay. I didn't look at the NWAM changes. Things look
> good; just some minor comments:
Thanks for the review.
> states.c:
>
> * 461-464: Seems like it'd be simpler to just check if dsm_ack !=
> NULL; is there a reason that's inappropriate?
Only that I didn't think of it. OK; I'll do that and retest the
dhcpinfo functionality.
> agent.c:
>
> * 522: Comment saying "we should report failure" seems odd given
> the subsequent sentence.
It means that this is what we should _generally_ do. But, ok, I'll
reword this.
/*
* If the user asked for the primary DHCP interface by giving
* an empty string and there is no primary, then check if we're
* handling dhcpinfo. If so, then simulate primary selection.
* Otherwise, report failure.
*/
> defaults.c:
>
> * 62: Indenting for "NULL" seems a bit off.
Fixed.
> dhcpagent_ipc.c:
>
> * 289: Seems like DHCP_IPC_WAIT_FOREVER would be more appropriate.
The documentation for this function says "-1 is forever". It looks to
me like DHCP_IPC_WAIT_* was intended to be a different data type --
those symbols are for functions that deal with seconds, not
milliseconds.
This function takes DHCP_IPC_DEFAULT_WAIT when the input is
DHCP_IPC_WAIT_DEFAULT. That implies to me that DHCP_IPC_WAIT_FOREVER
would be similarly inappropriate. (As far as I can tell, there are no
#defines that are useful as input to this function.)
I could change it, but it wouldn't look right to me. How strongly do
you feel about this?
> dhcpagent_util.c:
>
> * 86-108: Appears to be lifted from zoneadmd.c; perhaps an RFE is
> in-order for a utility function.
I'll file one. There are several others besides the one in zoneadmd,
and I didn't want to rototill the world for this change. :-/
> Then again, we should probably
> be using SMF for the DHCP client anyway.
Quite true; it should eventually just go away.
> * 174: Comment here seems odd; was "child to finish" meant?
It depends on your perspective. The "child" I care about here isn't
the pid I just forked, but rather the daemon that I've intentionally
started. I want to see that it has safely daemonized itself before I
do the abandon, so I wait for my own child pid to exit.
I agree that it's confusing, so I'll reword this to say "daemon to
run" instead.
> * 155-158: I'm a bit confused by this. Why not just put the
> contract_latest(&ct) call directly in the `default' arm of
> the swtich statement?
I'm just reducing code duplication. The contract_latest needs to be
done before ct_tmpl_clear and close, but those two things need to be
done for the -1 (failure) and 0 (child) cases as well.
--
James Carlson, Solaris Networking <james.d.carlson at sun.com>
Sun Microsystems / 35 Network Drive 71.232W Vox +1 781 442 2084
MS UBUR02-212 / Burlington MA 01803-2757 42.496N Fax +1 781 442 1677
