On 19.09.2014 18:13, Daniel P. Berrange wrote:
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.
I see.
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.
Right, this would be great.
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.
I was thinking about this prior to starting new implementation. The problem I had found myself in was that in secdrivers we use pure virDomainDefPtr but the VM's pid is stored in virDomainObjPtr. And I don't think we want to refactor all the code just to tunnel the vm->pid into function actually calling chown() or fsetfilecon_raw(). Or do we?
Michal -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list