Peter Memishian wrote:
> > 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.
>
OK, how about we use the following names (assuming all the functions
become part of libdladm):
For the Link ID management API:
dladm_create_linkid() /* get_new_linkid */
dladm_destroy_linkid() /* delete_linkid */
dladm_linkid2name() /* id2name */
dladm_name2linkid() /* name2id */
dladm_change_linkid_mapping() /* rename */
dladm_walk_linkid() /* The walker mentioned in my reply to Cathy */
For the persistent link configuration API:
dladm_create_conf() /* get_new_conf */
dladm_set_conf_field() /* set_conf_field */
dladm_get_conf_field() /* get_conf_field */
dladm_read_conf() /* get_conf */
dladm_write_conf() /* commit_conf */
dladm_delete_conf() /* delete_conf */
dladm_destroy_conf() /* free_conf */
dladm_walk_conf() /* walk_confs */
> 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?
>
I'd prefer to not require class, ID, and name for the create call,
because what if in the future we need more required fields in the
future, but I see your point. I'll add class, name, and ID as required
fields when you create a new configuration structure. I'll also make
the ID and class field read-only so that attempts to set either with
dladm_set_conf_field will fail.
> > 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()?
>
I could add some checks. We could have the commit check to make sure
the name to ID mapping in the configuration structure matches what the
link ID management API shows. However, I'm not sure how the
configuration API would tell the difference between this situation and a
rename operation.
For example, we search using an ID to see if there is already an
existing persistent configuration for this ID. Since net10 has net6's
ID, there is a configuration. We can tell the names are different, but
how does the API tell the difference between this operation and an
attempt to rename the interface?
Maybe I'm missing something.
> 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.
>
Another problem I thought of as I typed this up is what happens if
devfsadmd crashes and is restarted? Then we lose all the temporary link
ID to name mappings, unless devfsadmd has some way of storing the
temporary link ID to name mappings. If we persist them with the
persistence API, the mapping is restored on reboot, which is not what
we'd want.
I think my use of the term persistence could be problematic elsewhere.
> > 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.
>
OK
> > 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.
>
Even if we have two processes (or even two threads in the same process)
trying to change the same link's configuration?
Thanks,
Dan
