Thanks very much for your comments. Please see my replies inline: >> http://cr.grommit.com/~yun/webrev_uv/ > > General > ------- > > * There are a number of libdladm API functions that treat an input of > DATALINK_INVALID_LINKID as meaning that the function should operate on > all links. I'd prefer reserving a value for DATALINK_ALL_LINKID for this > purpose to not overload a constant which has quite different semantics. > Accepted. > > usr/src/uts/common/sys/dls.h > ---------------------------- > > * The DATALINK_MEDIAREQ_* family of macros seem unnecessarily complex. > It seems like this invention exists to save an extra argument to a > function. I think this can be simplified. > I've removed the GLDV3 part of this macro, and now the media argument is much simplified. Because the media type values are not bitwise, a higher 32 bits are still used to indicate ANY_MEDIA_TYPE semantics. But now the media argument is either a single media type value, or a DATALINK_ANY_MEDIATYPE macro.
> * The DATALINK_MEDIA_ISGLDV3() macro and its uses prevents unbundled > MAC-Type plugins to be added to the system by a 3rd party and having > things "just work" without recompilation of the OS. There is no way to > know at compile time what GLDv3 media types will be supported on the > running system. I haven't inventoried all of the places where this macro > is used, but I suspect this can be replaced with some dynamic mechanism > to detect if a given link is is a GLDv3 link. > I've answered this in another mail. > > usr/src/cmd/dladm/dladm.c > ------------------------- > > * When an alternate root is specified for any dladm subcommand, the > libdladm function must never operate on the active configuration. This > webrev doesn't show how altroot is used since that was putback after the > webrev was generated, but I believe this is a problem in the current code. > This has been done. > * 225-267: I'd move rename-link next to show-link in the usage, and I'd > move the *-phys subcommands up to where show-dev is. > Accepted. > * 439: nit: arg is the first argument. > Accepted. > > usr/src/cmd/dlmgmtd/dlmgmt*.* > ----------------------------- > > * One of these files needs some intro. comment explaining what this > daemon does and what is very high-level architecture is. > Accepted. I also accepted Meem's suggestion to split dlmgmt.c to several files. > > usr/src/lib/libdladm/common/libdladm.c > -------------------------------------- > > * 261-286: dladm_media2str() only prints something for three media types. > There are dozens more that could be included there. > Accepted. > * 402: This function is called by a surprising number of callers. In > creating a vlan or aggregation, it looks like this is called twice; once > in dladm_*_create(), and another in dlmgmt_create_common(). I would > think that you'd only need to validate a name when creating a linkid > (because you can't do anything else without a linkid), and when renaming > a link. > Accepted. > > usr/src/lib/libdladm/common/libdlaggr.c > --------------------------------------- > > * 142: Except for the open and close, this is identical to > i_dladm_ioctl(). We could call i_dladm_ioctl() in here instead. > Accepted. > > usr/src/uts/common/io/dld/dld_drv.c > ----------------------------------- > > * 584: This doesn't really rename anything, so the function name (and the > name of the associated ioctl DLDIOC_RENAME) is odd. It's more like > assigning a new linkid (DLDIOC_NEWLINKID/drv_ioc_newlinkid()). Same > comment for the dls_vlan_rename() function. > There is a bug there. The function needs to do the rename of the kstats. I kept the "rename" names as it really handles the renaming request. > * 584: I believe you're "leaking" entries in the ap_hashp here. It's not > a true leak since there is a reference to the ap information in the hash, > but once this linkid reassignment has happened, there is no way for > user-land to delete this information. I think you need to do a > mod_hash_remove() of the old linkid from ap_hashp in this function. You > may want to look for other cases like this where there are kernel data > structures referring to the old linkid. > It did not leak any ap_hashp, as in the dladm_rename_link() function, dladm_set_prop() is called to reset all the autopush configuration to empty. But I agree it is better to do it as part of the kernel renaming processing. > * 918: Why do we return -1 if a legacy dev_t is passed in? Maybe this > comment is not entirely accurate or misleading. We still allow autopush > above dld when a stream associated with a legacy device is opened, right? > You mention legacy devices in the third case as well (when you return > 1), so it's a bit confusing. > I've changed the comments to: "/* * Return 0 if this is dev_t of a GLDv3 data-link and we find its per-link * autopush configuration, update dlap in this case. The dev_t could be * any GLDv3 data-link, including softmac data-links, VLANs, aggrs etc. * Return 1 if this is dev_t of a GLDv3 physical data-link, but we failed to * find its per-link autopush configuration. In this case, we replace the * devp with the dev_t of the associating physical device, so that * stropen() can find the per-driver autopush setting for the real device. * In the case of a softmac data-link, the dev_t will be replaced with the * dev_t of the underlying legacy physical device. * Return -1 otherwise. This could be the case that if the dev_t is of a * legacy device, or the dev_t is of a GLDv3 non-physical data-link and we * cannot find its per-link autopush configuration. */ > > usr/src/uts/common/io/dls/dls_vlan.c > ------------------------------------ > > * 584: The comment here confuses me. This function doesn't do any > renaming. Moreover, case #1 does not involve this function at all if I > understand correctly. > Case #1 needs to check whether the dls_vlan_t is held. Also, as I mentioned above, it needs to update the name of the kstats. > > usr/src/uts/common/io/mac/mac.c > ------------------------------- > > * 441: Intro. comment is needed for mac_perstream_open(). > Added. > * 540: Rename to mac_is_held(). > Accepted. > * 545: Why is i_mac_impl_lock being entered here? The instant you > release the lock at 548, mi_ref could go down to 0, and "hold" will no > longer be accurate. > Accepted. > * 1405: Please add a comment explaining why we need all three methods of > assigning an instance number. Added. > * 1426: I don't believe that mi_minor is effectively used anywhere. In > every place I've looked, it is always used by subtracting 1 to get the > instance number. Why don't we just save the instance number and not > bother with mi_minor? > mi_minor is used to create the minor node. > * 1588-1600: I don't think it's right that mac is not responsible for > creating the link, but it is responsible for creating the DLPI device > node that gives access to the link. Why not have these > ddi_create_minor_node() calls in dls_create(), and the > ddi_remove_minor_node() calls in dls_destroy(). It's already done this > way for VLANs. > Unfortunately we cannot do it. As minor node creation is critical to the DACF framework. It must be done as part of mac_register() so that softmac_create() can be called as part of the post-attachment routine. The function is called following the below order: mac_register()->ddi_create_minor_node()->softmac_create()->dls_create() > > usr/src/uts/common/io/softmac/softmac_main.c > -------------------------------------------- > > * We need an intro. block comment explaining what softmac is and its > high-level architecture. > Added. Thanks again for your comments. - Cathy
