> > > The cscope is build in
> > >
> > > /net/lido.sfbay/export/home/cathy/onnv-bugfix/usr/src/
>
> http://cr.opensolaris.org/~yun/webrev_6729044_new/
>
> http://jurassic.sfbay/net/lido.sfbay/export/home/cathy/onnv-bugfix/webrev_internal_new/
>
> > cmd/syseventd/modules/datalink_mod/datalink_mod.c:
> >
> > * 36: I presume <string.h> was meant?
> >
> strcmp() is defined in string(3C) which is <strings.h>
As per the manpage, <strings.h> should only be used for strcasecmp() and
strncasecmp(). As per the C standard, strcmp() is in <string.h>.
> > * 39: Seems like <errno.h> should be included.
> >
> Added. Note that <sys/errno.h> is implicitly included by
> <sys/sysevent/eventdefs.h>.
Yes, I figured the code compiled somehow ;-)
> > * 70: Why 10 instead of SE_MAX_RETRY_LIMIT?
> >
> Changed.
>
> I was following the convention setup by zfs_mod.c etc. But it seems even
> we set 10 here, only SE_MAX_RETRY_LIMIT takes effect.
Indeed.
> > cmd/dlmgmtd/dlmgmt_door.c:
> >
> > * 77: Why define this function to return a value, then ignore
> > the return value? And why isn't this function `static'?
>
> Changed.
As an aside: would prefer to see the comment on lines 87-88 removed and
the comment at line 77 updated to state that it will be consumed by the
datalink sysevent module.
> >
> > * 94: Would prefer `progname' over a hardcoded "dlmgmtd".
> >
> Changed.
You don't need the `progname' extern you added since it's already in
dlmgmt_impl.h (and more generally, IMO an extern in a .c file is always
a bug).
> > * 208: s/notify/generates/ s/new_link/RCM_RESOURCE_LINK_NEW/
> >
> Changed.
Seems to say "generate" now instead of "generates". (Also, just a
curiosity: why the new `linkid' variable used on line 215?)
> > lib/libdladm/common/libdllink.h:
> >
> > * This constant seems out-of-place, both in name and purpose;
> > seems like it should be under <sys/sysevent/>. Also, the
> > comment says it's used for RCM, but it seems like that use
> > is covered by RCM_NV_LINKID.
> >
> What do you suggest then. Note that I am removing RCM_NV_LINKID, and this
> DATALINK_NV_LINKID constant need to be accessed both by xxx_rcm module and
> dlmgmt_door.c.
To my point of view, those constants are two separate things which happen
to be the same by implementation convenience (i.e., the datalink sysevent
module is smart enough to realize that it can just steal the nvpair rather
than creating a new one). Given that, I think the RCM_NV_LINKID constant
should remain and be used by all the RCM code, and by libdllink.c (since
it's generating an RCM event). That leaves just the dlmgmt_door.c call
site. Normally, I'd say this should use its own constant that's #defined
in e.g. <sys/sysevent/datalink.h>. But I'm not sure it's worth defining a
whole header file for one constant, especially one with the same value as
RCM_NV_LINKID. So I might cheat and just use RCM_NV_LINKID for now and
add <sys/sysevent/datalink.h> later if we enhance datalink sysevents.
> > uts/common/io/softmac/softmac_main.c:
> >
> > * Comment on lines 296-297 seems redundant given 276-277.
The use of "if" on line 285 seems weird since we can only get here if
it's true. Also, I think the comment can be made a bit more concise
(e.g. "note that" is not necessary). How about:
/*
* All of the minor nodes have been attached; start a taskq
* to do the rest of the work. We use a taskq instead of of
* doing the work here because:
*
* - We could be called as a result of an open() system call
* where spec_open() already SLOCKED the snode. Using a taskq
* sidesteps the risk that our ldi_open_by_dev() call would
* deadlock trying to set SLOCKED on the snode again.
*
* - The devfs design requires no interruptible function calls
* in the device post-attach routine, but we need to make an
* (interruptible) upcall. Using a taskq to make the upcall
* sidesteps this.
*/
> > * 417: We no longer explain the EBADF logic (and why does ENOENT
> > no longer apply?)
> >
> Changed.
On line 409 s/because the/be because/, and on line 410 s/Create/create.
> ENOENT was only possible when EBADF is returned by dls_mgmt_create(). And
> now, we do not call softmac_update_info() in that case.
I see.
--
meem
