Sebastien Roy wrote:
> Team,
>
> Since I've had my head buried in here for a little while, here's some
> preliminary input (i.e., preliminary to an actual code-review) on the
> linkmgmt door upcall.
>
> I'm trying to test my implicit iptun creation in /dev/net, and an issue
> I've run into is linkmgmtd's assumption that anything that has a
> non-zero VLAN-id has to automatically persist. Since IP tunnels have no
> VLAN id's, this assumption will cause implicit IP tunnel link-id's to
> persist when they shouldn't.
>
> The code in question is in linkmgmt.c:
>
> /*
> * Create both active and persistent link configuration for physical
> * links and only create active one for (implicitely created) VLANs.
> */
> flags = (create->ld_vid != VLAN_ID_NONE) ? LINKMGMT_ACTIVE :
> (LINKMGMT_ACTIVE | LINKMGMT_PERSIST);
>
> My feeling here is that the caller should determine whether he wants the
> configuration to persist or not, and I don't think that linkmgmtd should
> try and guess based on unrelated parameters. This could be resolved by
> having an additional flags field in the create structure which could
> take LINKMGMT_ACTIVE and/or LINKMGMT_PERSIST.
>
I did that only because from the existing input (the VLAN ID), it is already
enough information to derive whether this configuration needs to be
persistent or only temporary.
But as you said, we could add additional flag.
> Also, this code in linkmgmt.c linkmgmt_dls_create() (the same function
> as above) seems unnecessary:
>
> if (create->ld_vid != VLAN_ID_NONE) {
> class = DLADM_CLASS_VLAN;
> } else {
> switch (create->ld_media) {
> case DL_ETHER:
> class = DLADM_CLASS_ETHER;
> break;
> case DL_WIFI:
> class = DLADM_CLASS_WIFI;
> break;
> default:
> err = EINVAL;
> goto done;
> }
> }
>
> Why not have the caller of the upcall pass up the class instead of a
> media type? This is the only place where ld_media is referenced, so
> there doesn't seem to be a purpose for it other than to derive a class.
>
This is because "class" is a concept in dladm only. I know now its
definition is in dls.h. But I will move it to libdladm.h.
Thanks
- Cathy
