Re: [libvirt] [PATCH v1 07/10] lock_daemon: Implement seclabel APIs

On Wed, Sep 10, 2014 at 03:26:13PM +0200, Michal Privoznik wrote: > The most interesting part where all the hard work takes place. > > For storing triplets of strings a two dimensional table would be > the most intuitive approach. Or, if the triplet is rearranged > into pair of one string and a pair, a hash table can be used. But > hey, we already have those! Excellent. In other words, > > <path, model, seclabel> > > is turned into: > > <path, <model, seclabel>> > > The @path is then the key that the hash table is filled with. The > inner pair is turned into linked list (hash table in a hash table > would be awful really). Moreover, we need to store yet another > item: reference counter. I wonder, since 'model' is mandatory and (path, model) should be unique, could we not avoid the linked list complexity by using a string of 'model:path' as the hash key, then we only have a single hash item too and O(1) access to values ? > int > -virLockDaemonRememberSeclabel(virLockDaemonPtr lockd ATTRIBUTE_UNUSED, > - const char *path ATTRIBUTE_UNUSED, > - const char *model ATTRIBUTE_UNUSED, > - const char *seclabel ATTRIBUTE_UNUSED) > +virLockDaemonRememberSeclabel(virLockDaemonPtr lockd, > + const char *path, > + const char *model, > + const char *seclabel) > { > - /* Implement me */ > - return -1; > + int ret = -1; > + virSeclabelListPtr list = NULL; > + virSeclabelListPtr tmp = NULL; > + bool list_created = false; > + > + virMutexLock(&lockd->lock); > + > + if (!(list = virHashLookup(lockd->seclabels, path))) { > + if (VIR_ALLOC(list) < 0) > + goto cleanup; > + > + if (virHashAddEntry(lockd->seclabels, path, list) < 0) > + goto cleanup; > + > + list_created = true; > + } > + > + tmp = list; > + if (!(tmp = virSeclabelListFind(list, model))) { > + /* seclabel for @model doesn't exit yet */ > + if (!(tmp = virSeclabelListAdd(list, model, seclabel))) > + goto cleanup; > + } > + > + /* Update refcount */ > + ret = ++tmp->item->refcount; > + > + cleanup: > + virMutexUnlock(&lockd->lock); > + if (ret < 0 && list_created) { > + virHashRemoveEntry(lockd->seclabels, path); > + virSeclabelListFree(list); > + } > + return ret; > } What concerns me a little is that we don't have any association between the label, use count and the running VMs that triggered the need for the relabelling. Lets say that a virtualization host with VMs running crashes. The libvirtd and virtlocked on that host will loose all state. That's fine currently since you're not sharing the data across hosts. We're going to need to share the data though, so when we do that sharing, I think we're going to need to remember information about the VMs + hosts associated with the label, so that when a host crashes, we can have a chance of purging the now stale information.

Now, as discussed on IRC it isn't critical to support multi-host mode right away, but this does impact on the RPC protocol, since the messages we are passing do not include any information about the VM or host. So I'm thinking that this series should record the VM name + uuid and the host name + uuid against the seclabels and plumb that through the RPC protocol at least.

If we did that now, I think we might not even need to add new RPC protocol for lockd, and instead just re-use the existing protocol messages and at least some of the client/dispatcher code too. It would be more like a case of just setting up a new lockspace for recording info about labels, instead of a completely new set of infrastructure side-by-side.