> http://www.opensolaris.org/os/project/clearview/uv/link_id_management.pdf
Thanks for the revised document; here's my next round of feedback on
section 3:
* In general, a `flags' uint_t argument seems preferable to a
`persistent' boolean_t argument, since:
* We already take this approach with other libdladm
APIs (and already have DLADM_OPT_PERSIST).
* We can add DLADM_OPT_TEMP support to these APIs if
need be (this can be useful if the caller needs
more fine-grained error handling -- it can first
call DLADM_OPT_TEMP, then with DLADM_OPT_PERSIST).
* It's easily extensible.
* WiFi/GLDv3 added a dladm_set_rootdir() function. I'd prefer
we use this and remove any `altroot' function arguments.
* The handling of `name' in dladm_create_dlconf() seems imprecise.
For instance, it seems the caller cannot request *just* a
specific instance (e.g., "foo3"). Instead, what if:
* When `name' ends with a number, it be taken literally,
and the function fails if it's already in-use.
* When `name' does not end in a number, the function
selects the next available instance.
* The class_t and linkid_t data types are too general, and need
appropriate prefixes.
* Having to specify that a configuration be persisted both via
dladm_create_dlconf() and via dladm_commit_dlconf() seems odd.
Why is this necessary?
* Since we may have other types of IDs in the future,
dladm_get_linkid() seems preferable to dladm_get_id().
* I'm confused why dladm_destroy_dlconf() isn't called
dladm_destroy_linkid() -- it doesn't appear to operate on a
dlconf. Further, I'd expect dladm_release_dlconf() to be
dladm_destroy_dlconf(), since it appears to be what frees the
dlconf storage. I'm also unsure if dladm_commit_dlconf() still
has the side effect of doing the destroy operation -- I don't
see mention of it, but the examples in section 3.12 seem to
suggest it's there.
* The names dladm_get_dlconf() and dladm_set_dlconf() seem
misleading, as they do not get/set the entire dlconf().
These seem more like dladm_get_dlconf_field() and
dladm_set_dlconf_field(). With those changes,
dladm_retrieve_dlconf() could become dladm_get_dlconf(), which
seems semantically more accurate.
* How does one walk the just persistent or temporary links using
dladm_walk_link()?
* The `link' argument names should be renamed to `dlconf' for
consistency.
* The examples in section 3.2 don't seem completely updated --
e.g., the example shows dladm_create_dlconf() being called
with 3 arguments instead of 4.
* Argument/type naming nits (if you prefer your choices, then
certainly don't feel obligated to change):
dladm_dlconf_handle_t -> dladm_dlconf_t
buffer -> value
buffer_length -> vallen
--
meem
