Thanks. All the comments are accepted and the webrevs are updated. Thanks - Cathy
> > > > 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. >
