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.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|