hi meem
many thanks for the detailed review! I'm going
to work through your netcfgd code review comments
first if that's okay. We probably won't publish
a complete updated webrev for a little while since we want
to address as much of the feedback as possible, but
I've placed an updated webrev which includes these
changes at:
http://zhadum.east/export/ws/amaguire/nwam1-bugs/webrev/
I haven't addressed the issue of moving netcfgd to
usr/src/cmd/netcfgd here yet, though I agree it's a good
idea.
Peter Memishian wrote:
netcfgd:
* Unclear why this consists of multiple source files; it seems
like everything could be consolidated into a single source file
without sacrificing maintainability.
accepted - we just have netcfgd.c now.
netcfgd/Makefile:
* 29-30: Pulling in Makefile.lib seems wrong since that's for
building libraries. Just use $(ROOT)/lib.
done.
* 35: Remove needless HEADERS definition.
done.
* 61: What does this $(ROOTCMD) dependency do?
It's not needed.
netcfgd/util.h:
* Given that this only has logging functions, seems odd it's named
util.h. (But then again, I can't figure out why this exists.)
* 33: Need a /* PRINTFLIKE2 */ here.
* 39: No externs, please.
I've removed this file as per your comment above.
netcfgd/logging.c:
* 28-29: Unclear what this comment has to do with anything; no
debug.c here.
this was copy-and-paste cruft - a fair bit of
this crept into netcfgd I'm afraid.
* 42: s/The idea for having this function is so that you can
/This function allows you to/
* 43: s/Its/It's/
accepted.
* 59: Why 256? Why not vasprintf()?
accepted - I've also removed the thread id logging
since it's not useful for netcfgd.
* 81-90: Could be replaced with (in util.h)
#define dprintf(...) nlog(LOG_DEBUG, __VA_ARGS__)
We don't really need dprintf anyhow, we can just
use nlog(LOG_DEBUG, ...) so I've removed it.
netcfgd/main.c:
* General: I can't figure out what the zonename code is for;
we never seem to use the `zonename' variable. My nits below
assume that somehow it's supposed to do something.
cruft again from the nwamd -> netcfgd cut-and-paste
I'm afraid.
* 28: Comment should provide an overview of what this daemon does
on a technical level.
I've added this.
* 32, 34, 38, 39, 40, et al: Superfluous #includes
I think we need syslog.h (39), and ditto
for sys/stat.h and sys/types.h since we
call umask(2). We need sys/wait.h for
wait(2) and unistd.h for pause(2). libscf.h,
locale.h, priv.h, strings.h and zone.h aren't
needed though as far as I can see.
* 51, 52: No excuse for these being globals.
accepted - I've moved fg to main() and removed
zonename.
* 57-59: Unclear what this comment has to do with anything.
removed.
* 68: Where does this code lookup SMF properties?
It doesn't anymore.
* 70: Where does this code manage privileges?
Ditto - more cruft.
* 76: Is there a reason this uses LOG_NDELAY?
I don't think so - particularly since we call syslog()
immediately after openlog() in start_logging() now.
I've removed this.
* 79-113: Someone *really* needs to put this in a library :-(
Looks like this is cut line-for-line from Nevada nwamd/main.c
which in turn took it from pppoe/pppoed.c.
Agreed.
* 124: Prefer const.
* 126: No need to define `zoneid'; just inline the call to
getzoneid() int the getzonenamebyid() call.
* 132: Why is it correct to charge on with GLOBAL_ZONENAME in
this case?
I've removed the zone code since it's not needed.
* 141-142: Just curious: what exactly are we localizing? All of
the messages here go to syslog which is not localized.
nothing needs to be localized - I've removed the
POFILES definition from the Makefile and the
relevant headers. In fact, the parent directory
Makefile wasn't even including netcfgd in
MSGSUBDIRS.
* 145: Why is this LOG_INFO? Seems like LOG_DEBUG. Also, LOG_PID
was specified to openlog() so I'm not sure why this includes the
pid as well. Finally, the message should be generated in
start_logging(), not here.
accepted.
* 149-155: Indenting here is one level off.
fixed.
* 162: Comment states the obvious.
removed it.
* 166-168: Unclear why we want to ignore USR1/USR2/INT.
I suspect the reasoning was USR1/USR2 were being used
by nwamd to dump config at one point. I've removed these
3 settings of signal disposition.
* 170: Need to resolve this XXX for SIGTHAW.
This was another artifact of the copy-paste-from nwamd -
at the time, we weren't handling SIGTHAW there. Removed.
the XXX as not relevant.
Thanks again for the in-depth review!
Alan
_______________________________________________
networking-discuss mailing list
[email protected]
