> > 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?
Not very. > > * 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. Yes, but then I expected it to say "grandchild to safely daemonize" or somesuch. > I agree that it's confusing, so I'll reword this to say "daemon to > run" instead. OK. > > * 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. It was unclear to me why contract_latest() needed to be called before those two things while contract_abandon_id() could be done afterwards -- but I didn't dig into the libcontract API details. -- meem
