(cc'ing nwam-dev as the spam issues seem to have
gone away)
Sowmini.Varadhan at Sun.COM wrote:
>
> On (09/02/09 10:26), Alan Maguire wrote:
>
>> many thanks for the careful review! I'm going to
>> try and address the netcfgd and libnwam comments
>>
>
> Ok.. I'm satisifed with the response to any review comments not
> explicitly mentioned here...
>
>
great! I'll try and address the remaining issues below...
>>> - why is netcfgd under $SRC/cmd/cmd-inet/lib/netcfgd? Seems like this is
>>> parallel to dlmgmtd for layer 3, so to be symmetric
>>> with dlmgtd, shouldn't it be under $SRC/cmd/netcfgd/*? at the very least,
>>> it would make sense to have $SRC/cmd/cmd-inet/nwam/netcfgd and
>>> $SRC/cmd/cmd-inet/nwam/nwamd ($SRC/cmd/cmd-inet/lib is misleading,
>>> since
>>> I don't think these apps get installed in /lib), but I think the
>>> cleaner solution is
>>> $SRC/cmd/nwam/netcfgd
>>> $SRC/cmd/nwam/nwamd
>>> $SRC/cmd/nwam/common /* for common themes like daemonize() */
>>>
>>>
>> nwamd and netcfgd are installed under /lib, in /lib/inet.
>> Since nwamd already lived under cmd-inet/lib,
>> we went with the same location for netcfgd. I'm not sure
>> if it makes sense to move netcfgd under an "nwam"
>> subdirectory, given that it may be used in non-NWAM
>> scenarios in the future.
>>
>
> In that case they should follow the existing convention used by
> daemons like /lib/inet/in.ndpd and go under cmd/cmd-inet/usr.lib/*
> But I believe Seb also had comments in this area that need to be merged in?
>
>
Not sure, but I think this issue was resolved while I was away?
>> Given the current ARC case to support daemonize() in libc,
>> we'll probably be revisiting that post-integration, so it's
>> probably not worth eliminating the redundancy right now.
>>
>
> Ok.
>
>
>> Since there's likely to be a bit of flux with
>> these interfaces in the future, and given that
>> libnwam isn't needed by any other consumers
>> other than NWAM-specific tools, we've made
>>
>
> this may be true today, but we anticipate that this will change in
> the future, but I can understand your dilemma (since we have similar
> issues for Brussels). I'm ok with your proposed solution below (but
> could you please point at the specific files where the distinction
> is made clear)
>
>
>
I've added comments to libnwam_priv.h
describing the consumers of these interfaces.
>> libnwam project-private. As such, there's limited
>> scope to denote that certain functions are more
>> private than others. What I've tried to do is to
>> indicate this by splitting out the structures and
>> functions that are only used by nwamd and netcfgd for
>> backend door initialization/removal and event
>> queue management into libnwam_priv.h, utilized
>> by nwamd and netcfgd.
>>
>> The functions in question are:
>>
>> nwam_backend_init()
>> nwam_backend_fini()
>> nwam_event_send()
>> nwam_event_send_fini()
>> nwam_event_queue_init()
>> nwam_event_queue_fini()
>>
>
>
>>> - nwam_backend_init(): lines 349-360. dlmgmtd had some issues with the
>>> filesystem not being writable early in boot. How does nwamd work
>>> around that problem?
>>>
>>>
>> There are a few different aspects to this:
>>
>> - the door files are located under /etc/svc/volatile to allow
>> creation early in boot (before the root fs is writable)
>> - in nwamd, when we create or modify configuration
>> (which we will do principally on first boot post-upgrade),
>> there is retry logic that ensures we retry every couple
>> of seconds, dealing with the scenario of a readonly root
>> early in the boot process.
>>
>>> - if fattach() fails at line 382, we should door_revoke() the
>>>
>
> Ok. Are there any issues with diskless boot to be considered?
>
>
hmm, I'll have to think about this one. I'll consult
with the team and get back to you.
>>> usr/src/lib/libnwam/common/libnwam_files.c
>>> - value_add_escapes(): instead of repeatedly doing malloc/free around this
>>> function, the caller can pass in the output array with its size. another
>>> alternative is for value_add_escapes() to create out by doing a strdup(in)
>>>
>>>
>>>
>> Accepted - value_add_escapes() now takes "in" and "out" strings
>> as arguments, filling in the latter with the escaped version
>> of the former. We pass a char array to the function for "out" so
>> no malloc is needed to add escapes now.
>>
>
> Ok, but it wasn't clear to me (from looking at the code) how we avoid
> spilling past the end of each array (do we know, for example, that in
> will be null terminated?). If this is all safe, then some comments
> explaining why it is ok to use strlen instead of strnlen would be good.
> Otherwise, we should probably just bound the string operations by
> NWAM_MAX_VALUE_LEN..
>
>
I've added additional string length validation to the
nwam_value_create() function, since it's where the
nwam_value_t we're using here comes from (thanks
for reminding me - that's one area of validation I'd
missed!). That should ensure that the values are bounded
correctly. I've added commenbts to value_{add|remove}_escapes
to note why it's safe to use strlen().
>>> - nwam_enm_set_name(), nwam_enm_get_prop_type() also appear dead.
>>>
>> They aren't used in onnv, but are by the GUI.
>>
>
> Ok, maybe some comments explaining this would be good (so that some
> ON developer doesn't strip them out by accident)
>
>
good idea, I've added a comment at the top of libnwam.h
to this effect.
>>> usr/src/lib/libnwam/common/libnwam_object.c
>>>
>> I've used dbname for places where the configuration container
>> is specified and added comments before each function.
>>
>
> Ok, there seem to be nits, though e.g., multiple spaces before parent in
> line 45f of libnwam_object.c. Presumably 'hg nits' or 'hg pbchk' will
> catch this before push.
>
>
yep. I've fixed this one for now. Thanks!
webrev at
http://zhadum.east/export/ws/amaguire/nwam1-fixes/webrev
Alan
