Artem Kachitchkine wrote: > Testing is still in progress, minor changes might still happen and debug > printf's need to be removed, but overall it is ready for review. > > http://cr.opensolaris.org/~artem/pers-0421/ > > Anyone is welcome to provide constructive comments, gentle critique, > witty remarks and naughty limericks. Following folks might be especially > interested: > > Cathy and/or Seb: dlmgmtd, dls > Eric Cheng and/or Michael Lim: mac, dld, dls > Ted You: bge > Sowmini: mac, dld, libdladm, dladm, mdb > > I'd appreciate your inputs before next Tuesday, April 29th. > Hi Artem,
I haven't done my reviewing yet, but I have several general questions and would like some clarification before I continue my review: 1. Can you think of any non-physical link driver (or even softmac) could call mac_prop_init()s? I ask this question because it is impossible for pseudo links (aggr, VNIC, or even softmac) to provide the instance argument. That number is assigned by the GLDv3 framework. 2. Assume the code path mac_start()->mac_prop_load()->dls_mgmt_linkprop_init()-> dlmgmt_upcall_linkprop_init()->dlmgmt_getlink_by_dev() ... Note that dlmgmt_getlink_by_dev() can only be used for physical links, and even for physical links, the device name is not the same as the mac name (for example, softmacs). Therefore, when dlmgmt_getlink_by_dev() is called with mac_name, it will return ENOENT for softmacs and other non-physical links. The similar problem exists in dld_set_public_prop(). It assumes that mac name is the same as the device name. 3. I assume that you know that mac_prop_init() could fail when a new network interface is plugged in or you fresh-install a system. I guess since mac_prop_init() is used to apply the persistent link property configuration, those failures do not matter. But is a console message needed here if that fails (the bge_attach() function, line 3091)? 4. I don't see dladm init-linkprop in network/physical service anymore. How link properties like autopush can be applied after your changes? Below are some of my code review comments. Again, note that I haven't finish looking at all the code yet. mac.c - It does not look right to call mac_prop_update() for the ENOTSUP case (line 2952). - mac_get_prop() does not seem to be symmetric to mac_set_prop(). I guess the reason that DLD_PROP_FLAG_MAC_ONLY only applies to mac_set_prop() is because in that case, the property is already initialized in the underlying driver. But the current flag name is misleading because it seems that this property only needs to be known by the mac layer. Can we change the name of DLD_PROP_FLAG_MAC_ONLY to something like DLD_PROP_FLAG_DRIVER_IS_SET? linkprop.c - In dladm_init_linkprop() function, it does not seem right to take (flags | DLADM_OPT_PERSIST) as the last argument. Note the possible flags value here is either DLADM_OPT_PROP_MAC_ONLY or DLADM_OPT_PROP_MAC_NAME. Thanks - Cathy _______________________________________________ networking-discuss mailing list [email protected]
