Peter Memishian wrote: > > Can someone take a look of of the fixes of the following bugs: > > > > 6729044 race between network_rcm and softmac when a physical device is > > attached > > 6696737 ndi_devi_enter() can deadlock with stopped threads, hanging system > > 6729144 dlmgmtd process_db_write() leaks dlmgmt_link_t > > > > The webrev is here: > > > > http://cr.opensolaris.org/~yun/webrev_6729044/ > > > > If you have the swan access, this is the internal link: > > > > > http://jurassic.sfbay/net/lido.sfbay/export/home/cathy/onnv-bugfix/webrev_internal/ > > > > The cscope is build in > > > > /net/lido.sfbay/export/home/cathy/onnv-bugfix/usr/src/ > > This looks great. A few small things: > Thanks for your comments. the webrev is updated here:
http://cr.opensolaris.org/~yun/webrev_6729044_new/ http://jurassic.sfbay/net/lido.sfbay/export/home/cathy/onnv-bugfix/webrev_internal_new/ See more inline: > cmd/devfsadm/devfsadm.c: > > * After your changes, looks like the RCM_* and MISSING_SYMBOLS > #defines can be removed from cmd/devfsadm/message.h. > Accepted. > cmd/syseventd/modules/Makefile: > > * 37: List was previously alphabetically sorted. > Accepted. > lib/libdevinfo/device_info.h: > > * 182: Should probably keep a commented-out 0x00000020 here > so it's clear this flag value can be recycled. > Accepted. > 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> > * 39: Seems like <errno.h> should be included. > Added. Note that <sys/errno.h> is implicitly included by <sys/sysevent/eventdefs.h>. > * 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. > cmd/dlmgmtd/dlmgmt_db.c: > > * 651: s/we've/we're/ > Done. > 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'? > Change to return void > * 78: Prefer `const char *subclass'. > Accepted > * 88: Will this `goto done' work correctly if nvlist_alloc() > fails? Seems like the `nvl' passed into nvlist_free() > may be uninitialized. > Initialize nvl to NULL. > * 94: Would prefer `progname' over a hardcoded "dlmgmtd". > Changed. > * 207: s/datalink/the datalink/ > Changed. > * 208: s/notify/generates/ s/new_link/RCM_RESOURCE_LINK_NEW/ > Changed. > * 215: Is there a need for the sysevent to be generated while > the table is locked? If not, can we move the unlock up to > line 201? > Changed. > cmd/rcm_daemon/common/network_rcm.c: > > * 59-61: PROP_NV_DDI_NETWORK no longer needed. > Removed. > * 83-88: devfs_minor_data no longer needed. > Removed. > * 807-811: This comment needs to be revised (e.g., we no longer > pass up events for the resources here). > Changed. > 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. > lib/libdladm/common/libdlmgmt.h: > > * This file appears to be unchanged. > > uts/common/io/softmac/softmac_main.c: > > * Comment on lines 296-297 seems redundant given 276-277. > Changed the comments. > * 292-294: I'm confused by the comment here -- no upcalls when > the device is attached, or when it's being attached? > Changed the comments. > * 415: s/First provide/Provide/ > Changed. > * 417: We no longer explain the EBADF logic (and why does ENOENT > no longer apply?) > Changed. ENOENT was only possible when EBADF is returned by dls_mgmt_create(). And now, we do not call softmac_update_info() in that case. Thanks - Cathy
