Cathy Zhou wrote: > 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. >
I'll add a note that the user is allowed to pass class as NULL. Is it possible for a user to do this: a) Create a persistent link net0 with class a b) create a temporary link net0 with class b, which is then made active If so, then I think we need to keep flags as is so that the user can chose which link's information he wants. > 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. > That's an oversight, I added it. > 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). > I read the whole thread between you and Meem. I'm going to add some text about how to use dladm_set_rootdir. A summary of what I'll add is that the user should call dladm_set_rootdir every time the user wants to change the root directory of the configuration, including back to the default. > 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.) > The update collision I had in mind was if someone updated the configuration since the last read. Using your example above, if another process updated the persistent storage between the call to dladm_read_conf and the call to dladm_write_conf, dladm_write_conf would return DLADM_STATUS_COLLISION. I'll have the function return DLADM_STATUS_EXIST for the case where the user is trying to persist a configuration that would cause a mapping collision. > 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. > OK, done. > 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. > Both changes were made. > 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. > I'll keep it for symmetry. thanks, Dan
