[brussels-dev] [networking-discuss] code review Brussels persistence

[Cathy Zhou]

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