> Here's how I fleshed it out. Any thoughts would be appreciated,
> especially on what the names should be.
The proposal seems generally reasonable to me; some thoughts follow.
First, regarding names: "get_new" and "delete" seem like an odd pair; I'd
vote for either alloc/free or create/destroy. (Those that created our CLI
guidelines believed create/delete are a pair, but to me "insert" is the
opposite of "delete".) Also, "get_conf()" seems more natural to me as
"read_conf()" and "commit_conf()" as "write_conf() -- but it's up to you.
While I agree that modeling the class, name, and id as regular fields
provides uniformity, I think it also increases the number of gestures
needed to use the API -- e.g., all code creating a new configuration will
have the same four-function-call boilerplate, and the same error-handling.
As such, I'd rather have the required fields be directly specified to the
function that allocates the new configuration, even if we also allow them
to be manipulated separately through a field-based API.
I'm also not sure I understand the rules associated with setting
(changing) the class and id fields for an existing configuration -- are
those operations allowed? If so, how would those changes be reflected in
the current configuration after changing the persistent configuration?
> To delete a link:
>
> delete_linkid("net6");
> delete_conf("net6"); /* If the link has been persisted */
>
> I see this as a potential problem. Both steps have to be done if the
> link has been persisted. If the API user only does the delete_linkid,
> then the link ID to name mapping is removed, but it still exists in
> persistent storage. For example:
>
> delete_linkid("net6");
> id = get_new_linkid("net10", class); /* Could return net6's link ID! */
>
> If the user persisted this new link ID, we'd have a link ID collision on
> the next reboot.
Why couldn't that be detected when the persistence is attempted -- e.g.,
fail the commit/write()?
Speaking of persistence: I notice you have been using the accepted
dladm(1M) definition of temporary meaning "not persisting across a
reboot". However, dynamic reconfiguration presents a challenge here, as
in general, state is not persisted across a dynamic reconfiguration event
either (e.g., IP configuration is reloaded from /etc/hostname.<if> when
the "cfgadm -c configure" is done).
While I'm sure it's possible to devise ways to ensure temporary link
configuration survives across a DR event, I don't think it's worth the
complexity, especially since future work may allow us to handle it "for
free". For instance, we've spoken in the past about follow-on work to
vanity naming that would allow us to swap the underlying hardware without
tearing down the current link configuration. So, it would be nice if we
could keep the current implementation simple while allowing us to revise
the "temporary" behavior in the future to survive across dynamic
reconfiguration. I think this will require some weasel-word updates to
the temporary semantics documented in dladm(1M), though.
> If the API user just calls delete_conf without delete_linkid, that
> situation is less harmful. On reboot, since there is no persistent
> configuration for net6, net6's link ID to name mapping is not restored
> and net6's ID is now available.
That sounds fine.
> One thing I do not address here is locking. We should have a mutex lock
> around the persistent configuration calls. I am not sure we need
> special locking for the link ID management calls, I think the locking
> you already have in place in devfsadmd would be sufficient. What do you
> think?
I don't see a need for explicit locking in the new API, which is nice.
--
meem
