Dan Groves wrote:
> I posted a new copy of the link ID management document to
>
> http://www.opensolaris.org/os/project/clearview/uv/link_id_management.pdf
>
> It includes modifications for all comments received so far except for
> the request from Cathy about a way to get at class information for a
> temporary link. I'll add changes for that after she's responded to my
> proposal.
>
Dan,
I looked through the recent version, and here are some minor comments:
a. dladm_linkid2info dladm_status_t dladm_linkid2info(dladm_linkid_t id,
uint_t flags, dladm_class_t *classp, char *name, size_t name_length)
I would think flags should also be a pointer: "uint_t *flagp", which returns
whether this link is persistent or active or both?
and I'd recommend that we allow the caller to set the flagp, and classp to
be NULL, which means the caller is not interested in the returned flags and
class - as this is the most common case.
b. dladm_walk_linkid()
I would think the fn() function would return either DLADM_WALK_CONTINUE or
DLADM_WALK_TERMINATE to indicate the walker function whether needs to keep
walking.
c. dladm_set_rootdir()
What's the signature of dladm_set_rootdir()?
"If the API s user needs to change configuration in another away (for
example, for a Jumpstart server) then the API user should call dladm set
rootdir to set a new root for the configuration."
So after set the new root, does the user need to set it back? After think
this more, I really think to have an extra function to set altroot adds more
complexities of the API. It would be more simpler if dladm_write_conf() and
dladm_remove_conf() to take altroot as an argument (if altroot is NULL,
means the default rootdir).
d. dladm_write_conf()
What exactly do you mean an update collision? Does it strictly check
<linkname, linkid> mapping collision or it checks other attribute as well?
For example, the processing of a modify-aggr subcommand would be:
dladm_read_conf(linkid, &conf);
dladm_set_conf_field(conf, FPOLICY, &attrp->ld_policy,
DLADM_TYPE_UINT32);
dladm_write_conf(conf);
Is that enough? Will it cause update collision? (because the link and
specific conf field already exist and this is an update operation.)
e. I'd suggest to use uint32_t as the type of flags instead of uint_t. No
particular reason, only because in the current code, it is uint32_t.
f. I'd think there is at least one more dladm_type_t: DLADM_TYPE_UINT64, and
the name of structure to be dladm_datatype_t? To not be confused with the
link type.
g. I am not sure whether dlamd_name2info() is necessary -
dladm_name2linkid() should be enough because we mostly operate on linkid.
But if you want to keep it, it is fine.
Thanks
- Cathy
