[libvirt] [PATCH v1 0/6] Introduce metadata locking
All patches are for: https://bugzilla.redhat.com/show_bug.cgi?id=1524792 You can find them on my Github too: https://github.com/zippy2/libvirt/tree/disk_metadata_lock_alt The locking by itself make very little sense. I have a patches waiting that implement restoring original seclabels using XATTRs, that should go on the top of these and can be also found on my Github: https://github.com/zippy2/libvirt/tree/seclabels But those are for different bug therefore I'll send them separately. And a bit of warning, I haven't bothered implementing this feature into sanlock driver. The main reason for that was that we tell users to prefer virtlockd over sanlock and if we really want them to switch lets do it by the former offering more features than the latter. Michal Prívozník (6): virlockspace: Introduce VIR_LOCK_SPACE_ACQUIRE_METADATA flag lock_driver.h: Introduce metadata flag lockd_driver_lockd: Implement metadata flag lock_driver_sanlock: Handle metadata flag gracefully domain_lock: Implement metadata locking qemu_security: Lock metadata while relabelling src/libvirt_private.syms | 8 + src/locking/domain_lock.c | 304 ++++++++++++++++++++++++++++++++++--- src/locking/domain_lock.h | 28 ++++ src/locking/lock_daemon_dispatch.c | 5 +- src/locking/lock_driver.h | 2 + src/locking/lock_driver_lockd.c | 31 ++-- src/locking/lock_driver_lockd.h | 1 + src/locking/lock_driver_sanlock.c | 25 ++- src/qemu/qemu_security.c | 107 +++++++++++++ src/util/virlockspace.c | 40 ++++- src/util/virlockspace.h | 1 + 11 files changed, 504 insertions(+), 48 deletions(-) -- 2.16.4
This flag is going to be used to alter default behaviour of the lock. Firstly, it means we will lock different offset in the file (offset 1 instead of 0). Secondly, it means the lock acquiring will actually wait for the lock to be set (compared to default behaviour which is set or error out). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virlockspace.c | 40 +++++++++++++++++++++++++++++++++------- src/util/virlockspace.h | 1 + 2 files changed, 34 insertions(+), 7 deletions(-) diff --git a/src/util/virlockspace.c b/src/util/virlockspace.c index 3364c843aa..a7ec0c05bf 100644 --- a/src/util/virlockspace.c +++ b/src/util/virlockspace.c @@ -111,6 +111,8 @@ static void virLockSpaceResourceFree(virLockSpaceResourcePtr res) VIR_FREE(res); } +#define DEFAULT_OFFSET 0 +#define METADATA_OFFSET 1 static virLockSpaceResourcePtr virLockSpaceResourceNew(virLockSpacePtr lockspace, @@ -120,6 +122,18 @@ virLockSpaceResourceNew(virLockSpacePtr lockspace, { virLockSpaceResourcePtr res; bool shared = !!(flags & VIR_LOCK_SPACE_ACQUIRE_SHARED); + bool metadata = !!(flags & VIR_LOCK_SPACE_ACQUIRE_METADATA); + off_t start = DEFAULT_OFFSET; + bool waitForLock = false; + + if (metadata) { + /* We want the metadata lock to act like pthread mutex. + * This means waiting for the lock to be acquired. */ + waitForLock = true; + + /* Also, we are locking different offset. */ + start = METADATA_OFFSET; + } if (VIR_ALLOC(res) < 0) return NULL; @@ -157,7 +171,7 @@ virLockSpaceResourceNew(virLockSpacePtr lockspace, goto error; } - if (virFileLock(res->fd, shared, 0, 1, false) < 0) { + if (virFileLock(res->fd, shared, start, 1, waitForLock) < 0) { if (errno == EACCES || errno == EAGAIN) { virReportError(VIR_ERR_RESOURCE_BUSY, _("Lockspace resource '%s' is locked"), @@ -204,7 +218,7 @@ virLockSpaceResourceNew(virLockSpacePtr lockspace, goto error; } - if (virFileLock(res->fd, shared, 0, 1, false) < 0) { + if (virFileLock(res->fd, shared, start, 1, waitForLock) < 0) { if (errno == EACCES || errno == EAGAIN) { virReportError(VIR_ERR_RESOURCE_BUSY, _("Lockspace resource '%s' is locked"), @@ -621,7 +635,15 @@ int virLockSpaceAcquireResource(virLockSpacePtr lockspace, lockspace, resname, flags, (unsigned long long)owner); virCheckFlags(VIR_LOCK_SPACE_ACQUIRE_SHARED | - VIR_LOCK_SPACE_ACQUIRE_AUTOCREATE, -1); + VIR_LOCK_SPACE_ACQUIRE_AUTOCREATE | + VIR_LOCK_SPACE_ACQUIRE_METADATA, -1); + + if (flags & VIR_LOCK_SPACE_ACQUIRE_METADATA && + flags & VIR_LOCK_SPACE_ACQUIRE_SHARED) { + virReportInvalidArg(flags, "%s", + _("metadata and shared are mutually exclusive")); + return -1; + } virMutexLock(&lockspace->lock); @@ -635,10 +657,14 @@ int virLockSpaceAcquireResource(virLockSpacePtr lockspace, goto done; } - virReportError(VIR_ERR_RESOURCE_BUSY, - _("Lockspace resource '%s' is locked"), - resname); - goto cleanup; + + if (!(res->flags & VIR_LOCK_SPACE_ACQUIRE_METADATA) || + !(flags & VIR_LOCK_SPACE_ACQUIRE_METADATA)) { + virReportError(VIR_ERR_RESOURCE_BUSY, + _("Lockspace resource '%s' is locked"), + resname); + goto cleanup; + } } if (!(res = virLockSpaceResourceNew(lockspace, resname, flags, owner))) diff --git a/src/util/virlockspace.h b/src/util/virlockspace.h index 041cf20396..a9a3b2e73f 100644 --- a/src/util/virlockspace.h +++ b/src/util/virlockspace.h @@ -45,6 +45,7 @@ int virLockSpaceDeleteResource(virLockSpacePtr lockspace, typedef enum { VIR_LOCK_SPACE_ACQUIRE_SHARED = (1 << 0), VIR_LOCK_SPACE_ACQUIRE_AUTOCREATE = (1 << 1), + VIR_LOCK_SPACE_ACQUIRE_METADATA = (1 << 2), } virLockSpaceAcquireFlags; int virLockSpaceAcquireResource(virLockSpacePtr lockspace, -- 2.16.4
On Thu, Aug 09, 2018 at 03:34:39PM +0200, Michal Privoznik wrote:
This flag is going to be used to alter default behaviour of the lock. Firstly, it means we will lock different offset in the file (offset 1 instead of 0). Secondly, it means the lock acquiring will actually wait for the lock to be set (compared to default behaviour which is set or error out). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virlockspace.c | 40 +++++++++++++++++++++++++++++++++------- src/util/virlockspace.h | 1 + 2 files changed, 34 insertions(+), 7 deletions(-)
diff --git a/src/util/virlockspace.c b/src/util/virlockspace.c index 3364c843aa..a7ec0c05bf 100644 --- a/src/util/virlockspace.c +++ b/src/util/virlockspace.c @@ -111,6 +111,8 @@ static void virLockSpaceResourceFree(virLockSpaceResourcePtr res) VIR_FREE(res); }
+#define DEFAULT_OFFSET 0 +#define METADATA_OFFSET 1
static virLockSpaceResourcePtr virLockSpaceResourceNew(virLockSpacePtr lockspace, @@ -120,6 +122,18 @@ virLockSpaceResourceNew(virLockSpacePtr lockspace, { virLockSpaceResourcePtr res; bool shared = !!(flags & VIR_LOCK_SPACE_ACQUIRE_SHARED); + bool metadata = !!(flags & VIR_LOCK_SPACE_ACQUIRE_METADATA); + off_t start = DEFAULT_OFFSET; + bool waitForLock = false; + + if (metadata) { + /* We want the metadata lock to act like pthread mutex. + * This means waiting for the lock to be acquired. */ + waitForLock = true; + + /* Also, we are locking different offset. */ + start = METADATA_OFFSET; + }
if (VIR_ALLOC(res) < 0) return NULL; @@ -157,7 +171,7 @@ virLockSpaceResourceNew(virLockSpacePtr lockspace, goto error; }
- if (virFileLock(res->fd, shared, 0, 1, false) < 0) { + if (virFileLock(res->fd, shared, start, 1, waitForLock) < 0) { if (errno == EACCES || errno == EAGAIN) { virReportError(VIR_ERR_RESOURCE_BUSY, _("Lockspace resource '%s' is locked"), @@ -204,7 +218,7 @@ virLockSpaceResourceNew(virLockSpacePtr lockspace, goto error; }
- if (virFileLock(res->fd, shared, 0, 1, false) < 0) { + if (virFileLock(res->fd, shared, start, 1, waitForLock) < 0) { if (errno == EACCES || errno == EAGAIN) { virReportError(VIR_ERR_RESOURCE_BUSY, _("Lockspace resource '%s' is locked"), @@ -621,7 +635,15 @@ int virLockSpaceAcquireResource(virLockSpacePtr lockspace, lockspace, resname, flags, (unsigned long long)owner);
virCheckFlags(VIR_LOCK_SPACE_ACQUIRE_SHARED | - VIR_LOCK_SPACE_ACQUIRE_AUTOCREATE, -1); + VIR_LOCK_SPACE_ACQUIRE_AUTOCREATE | + VIR_LOCK_SPACE_ACQUIRE_METADATA, -1); + + if (flags & VIR_LOCK_SPACE_ACQUIRE_METADATA && + flags & VIR_LOCK_SPACE_ACQUIRE_SHARED) { + virReportInvalidArg(flags, "%s", + _("metadata and shared are mutually exclusive")); + return -1; + }
virMutexLock(&lockspace->lock);
@@ -635,10 +657,14 @@ int virLockSpaceAcquireResource(virLockSpacePtr lockspace,
goto done; } - virReportError(VIR_ERR_RESOURCE_BUSY, - _("Lockspace resource '%s' is locked"), - resname); - goto cleanup; + + if (!(res->flags & VIR_LOCK_SPACE_ACQUIRE_METADATA) || + !(flags & VIR_LOCK_SPACE_ACQUIRE_METADATA)) { + virReportError(VIR_ERR_RESOURCE_BUSY, + _("Lockspace resource '%s' is locked"), + resname); + goto cleanup; + } }
if (!(res = virLockSpaceResourceNew(lockspace, resname, flags, owner))) diff --git a/src/util/virlockspace.h b/src/util/virlockspace.h index 041cf20396..a9a3b2e73f 100644 --- a/src/util/virlockspace.h +++ b/src/util/virlockspace.h @@ -45,6 +45,7 @@ int virLockSpaceDeleteResource(virLockSpacePtr lockspace, typedef enum { VIR_LOCK_SPACE_ACQUIRE_SHARED = (1 << 0), VIR_LOCK_SPACE_ACQUIRE_AUTOCREATE = (1 << 1), + VIR_LOCK_SPACE_ACQUIRE_METADATA = (1 << 2), } virLockSpaceAcquireFlags;
int virLockSpaceAcquireResource(virLockSpacePtr lockspace,
The virLockSpace code is intended to be a bit more generic than the locking protocol. So I'd have a preference for having a VIR_LOCK_SPACE_ACQUIRE_WAIT flag, and passing in the offset + length to the APIs as parameters. Just have the "METADATA" concept in the lock driver only, not this general purpose code. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/locking/lock_driver.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/locking/lock_driver.h b/src/locking/lock_driver.h index 8b7cccc521..7c8f79520a 100644 --- a/src/locking/lock_driver.h +++ b/src/locking/lock_driver.h @@ -56,6 +56,8 @@ typedef enum { VIR_LOCK_MANAGER_RESOURCE_READONLY = (1 << 0), /* The resource is assigned in shared, writable mode */ VIR_LOCK_MANAGER_RESOURCE_SHARED = (1 << 1), + /* The resource is locked for metadata change */ + VIR_LOCK_MANAGER_RESOURCE_METADATA = (1 << 2), } virLockManagerResourceFlags; typedef enum { -- 2.16.4
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/locking/lock_daemon_dispatch.c | 5 ++++- src/locking/lock_driver_lockd.c | 31 +++++++++++++++++++++---------- src/locking/lock_driver_lockd.h | 1 + 3 files changed, 26 insertions(+), 11 deletions(-) diff --git a/src/locking/lock_daemon_dispatch.c b/src/locking/lock_daemon_dispatch.c index 1b479db55d..ff5a33fa33 100644 --- a/src/locking/lock_daemon_dispatch.c +++ b/src/locking/lock_daemon_dispatch.c @@ -54,7 +54,8 @@ virLockSpaceProtocolDispatchAcquireResource(virNetServerPtr server ATTRIBUTE_UNU virMutexLock(&priv->lock); virCheckFlagsGoto(VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_SHARED | - VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE, cleanup); + VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE | + VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_METADATA, cleanup); if (priv->restricted) { virReportError(VIR_ERR_OPERATION_DENIED, "%s", @@ -80,6 +81,8 @@ virLockSpaceProtocolDispatchAcquireResource(virNetServerPtr server ATTRIBUTE_UNU newFlags |= VIR_LOCK_SPACE_ACQUIRE_SHARED; if (flags & VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE) newFlags |= VIR_LOCK_SPACE_ACQUIRE_AUTOCREATE; + if (flags & VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_METADATA) + newFlags |= VIR_LOCK_SPACE_ACQUIRE_METADATA; if (virLockSpaceAcquireResource(lockspace, args->name, diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c index 957a963a7b..bd14ed8930 100644 --- a/src/locking/lock_driver_lockd.c +++ b/src/locking/lock_driver_lockd.c @@ -475,9 +475,11 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock, bool autoCreate = false; virCheckFlags(VIR_LOCK_MANAGER_RESOURCE_READONLY | - VIR_LOCK_MANAGER_RESOURCE_SHARED, -1); + VIR_LOCK_MANAGER_RESOURCE_SHARED | + VIR_LOCK_MANAGER_RESOURCE_METADATA, -1); - if (flags & VIR_LOCK_MANAGER_RESOURCE_READONLY) + if (flags & VIR_LOCK_MANAGER_RESOURCE_READONLY && + !(flags & VIR_LOCK_MANAGER_RESOURCE_METADATA)) return 0; switch (type) { @@ -489,7 +491,8 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock, } if (!driver->autoDiskLease) { if (!(flags & (VIR_LOCK_MANAGER_RESOURCE_SHARED | - VIR_LOCK_MANAGER_RESOURCE_READONLY))) + VIR_LOCK_MANAGER_RESOURCE_READONLY | + VIR_LOCK_MANAGER_RESOURCE_METADATA))) priv->hasRWDisks = true; return 0; } @@ -602,6 +605,10 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock, priv->resources[priv->nresources-1].flags |= VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE; + if (flags & VIR_LOCK_MANAGER_RESOURCE_METADATA) + priv->resources[priv->nresources-1].flags |= + VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_METADATA; + return 0; error: @@ -626,12 +633,15 @@ static int virLockManagerLockDaemonAcquire(virLockManagerPtr lock, virCheckFlags(VIR_LOCK_MANAGER_ACQUIRE_REGISTER_ONLY | VIR_LOCK_MANAGER_ACQUIRE_RESTRICT, -1); - if (priv->nresources == 0 && - priv->hasRWDisks && - driver->requireLeaseForDisks) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Read/write, exclusive access, disks were present, but no leases specified")); - return -1; + if (priv->nresources == 0) { + if (priv->hasRWDisks && + driver->requireLeaseForDisks) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Read/write, exclusive access, disks were present, but no leases specified")); + return -1; + } + + return 0; } if (!(client = virLockManagerLockDaemonConnect(lock, &program, &counter))) @@ -711,7 +721,8 @@ static int virLockManagerLockDaemonRelease(virLockManagerPtr lock, args.flags &= ~(VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_SHARED | - VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE); + VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE | + VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_METADATA); if (virNetClientProgramCall(program, client, diff --git a/src/locking/lock_driver_lockd.h b/src/locking/lock_driver_lockd.h index 6931fe7425..9882793260 100644 --- a/src/locking/lock_driver_lockd.h +++ b/src/locking/lock_driver_lockd.h @@ -25,6 +25,7 @@ enum virLockSpaceProtocolAcquireResourceFlags { VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_SHARED = (1 << 0), VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE = (1 << 1), + VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_METADATA = (1 << 2), }; #endif /* __VIR_LOCK_DRIVER_LOCKD_H__ */ -- 2.16.4
No real support implemented here. But hey, at least we will not fail. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/locking/lock_driver_sanlock.c | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index 3e5f0e37b0..c1996fb937 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -791,7 +791,8 @@ static int virLockManagerSanlockAddResource(virLockManagerPtr lock, virLockManagerSanlockPrivatePtr priv = lock->privateData; virCheckFlags(VIR_LOCK_MANAGER_RESOURCE_READONLY | - VIR_LOCK_MANAGER_RESOURCE_SHARED, -1); + VIR_LOCK_MANAGER_RESOURCE_SHARED | + VIR_LOCK_MANAGER_RESOURCE_METADATA, -1); if (priv->res_count == SANLK_MAX_RESOURCES) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -804,6 +805,11 @@ static int virLockManagerSanlockAddResource(virLockManagerPtr lock, if (flags & VIR_LOCK_MANAGER_RESOURCE_READONLY) return 0; + /* No metadata locking support for now. + * TODO: implement it. */ + if (flags & VIR_LOCK_MANAGER_RESOURCE_METADATA) + return 0; + switch (type) { case VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK: if (driver->autoDiskLease) { @@ -953,12 +959,17 @@ static int virLockManagerSanlockAcquire(virLockManagerPtr lock, virCheckFlags(VIR_LOCK_MANAGER_ACQUIRE_RESTRICT | VIR_LOCK_MANAGER_ACQUIRE_REGISTER_ONLY, -1); - if (priv->res_count == 0 && - priv->hasRWDisks && - driver->requireLeaseForDisks) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Read/write, exclusive access, disks were present, but no leases specified")); - return -1; + if (priv->res_count == 0) { + if (priv->hasRWDisks && + driver->requireLeaseForDisks) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Read/write, exclusive access, disks were present, but no leases specified")); + return -1; + } + + /* We are not handling METADATA flag yet. So no resources + * case is no-op for now. */ + return 0; } /* We only initialize 'sock' if we are in the real -- 2.16.4
In order for our drivers to lock resources for metadata change we need set of new APIs. Fortunately, we don't have to care about every possible device a domain can have. We care only about those which can live on a network filesystem and hence can be accessed by multiple daemons at the same time. These devices are covered in virDomainLockMetadataLock() and only a small fraction of those can be hotplugged (covered in the rest of the introduced APIs). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 8 ++ src/locking/domain_lock.c | 304 ++++++++++++++++++++++++++++++++++++++++++---- src/locking/domain_lock.h | 28 +++++ 3 files changed, 317 insertions(+), 23 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 32ed5a09f9..fdc45e724c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1284,6 +1284,14 @@ virDomainLockImageAttach; virDomainLockImageDetach; virDomainLockLeaseAttach; virDomainLockLeaseDetach; +virDomainLockMetadataDiskLock; +virDomainLockMetadataDiskUnlock; +virDomainLockMetadataImageLock; +virDomainLockMetadataImageUnlock; +virDomainLockMetadataLock; +virDomainLockMetadataMemLock; +virDomainLockMetadataMemUnlock; +virDomainLockMetadataUnlock; virDomainLockProcessInquire; virDomainLockProcessPause; virDomainLockProcessResume; diff --git a/src/locking/domain_lock.c b/src/locking/domain_lock.c index 705b132457..19a097fb25 100644 --- a/src/locking/domain_lock.c +++ b/src/locking/domain_lock.c @@ -69,7 +69,8 @@ static int virDomainLockManagerAddLease(virLockManagerPtr lock, static int virDomainLockManagerAddImage(virLockManagerPtr lock, - virStorageSourcePtr src) + virStorageSourcePtr src, + bool metadataOnly) { unsigned int diskFlags = 0; int type = virStorageSourceGetActualType(src); @@ -82,10 +83,14 @@ static int virDomainLockManagerAddImage(virLockManagerPtr lock, type == VIR_STORAGE_TYPE_DIR)) return 0; - if (src->readonly) - diskFlags |= VIR_LOCK_MANAGER_RESOURCE_READONLY; - if (src->shared) - diskFlags |= VIR_LOCK_MANAGER_RESOURCE_SHARED; + if (metadataOnly) { + diskFlags = VIR_LOCK_MANAGER_RESOURCE_METADATA; + } else { + if (src->readonly) + diskFlags |= VIR_LOCK_MANAGER_RESOURCE_READONLY; + if (src->shared) + diskFlags |= VIR_LOCK_MANAGER_RESOURCE_SHARED; + } VIR_DEBUG("Add disk %s", src->path); if (virLockManagerAddResource(lock, @@ -101,13 +106,68 @@ static int virDomainLockManagerAddImage(virLockManagerPtr lock, } +static int +virDomainLockManagerAddMemory(virLockManagerPtr lock, + const virDomainMemoryDef *mem) +{ + const char *path = NULL; + + switch ((virDomainMemoryModel) mem->model) { + case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: + path = mem->nvdimmPath; + break; + + case VIR_DOMAIN_MEMORY_MODEL_DIMM: + case VIR_DOMAIN_MEMORY_MODEL_LAST: + case VIR_DOMAIN_MEMORY_MODEL_NONE: + break; + } + + if (!path) + return 0; + + VIR_DEBUG("Adding memory %s", path); + if (virLockManagerAddResource(lock, + VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK, + path, + 0, + NULL, + VIR_LOCK_MANAGER_RESOURCE_METADATA) < 0) + return -1; + + return 0; +} + + +static int +virDomainLockManagerAddFile(virLockManagerPtr lock, + const char *file) +{ + if (!file) + return 0; + + VIR_DEBUG("Adding file %s", file); + if (virLockManagerAddResource(lock, + VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK, + file, + 0, + NULL, + VIR_LOCK_MANAGER_RESOURCE_METADATA) < 0) + return -1; + + return 0; +} + + static virLockManagerPtr virDomainLockManagerNew(virLockManagerPluginPtr plugin, const char *uri, virDomainObjPtr dom, bool withResources, + bool metadataOnly, unsigned int flags) { virLockManagerPtr lock; + const virDomainDef *def = dom->def; size_t i; virLockManagerParam params[] = { { .type = VIR_LOCK_MANAGER_PARAM_TYPE_UUID, @@ -115,11 +175,11 @@ static virLockManagerPtr virDomainLockManagerNew(virLockManagerPluginPtr plugin, }, { .type = VIR_LOCK_MANAGER_PARAM_TYPE_STRING, .key = "name", - .value = { .str = dom->def->name }, + .value = { .str = def->name }, }, { .type = VIR_LOCK_MANAGER_PARAM_TYPE_UINT, .key = "id", - .value = { .iv = dom->def->id }, + .value = { .iv = def->id }, }, { .type = VIR_LOCK_MANAGER_PARAM_TYPE_UINT, .key = "pid", @@ -133,7 +193,7 @@ static virLockManagerPtr virDomainLockManagerNew(virLockManagerPluginPtr plugin, VIR_DEBUG("plugin=%p dom=%p withResources=%d", plugin, dom, withResources); - memcpy(params[0].value.uuid, dom->def->uuid, VIR_UUID_BUFLEN); + memcpy(params[0].value.uuid, def->uuid, VIR_UUID_BUFLEN); if (!(lock = virLockManagerNew(virLockManagerPluginGetDriver(plugin), VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN, @@ -144,19 +204,46 @@ static virLockManagerPtr virDomainLockManagerNew(virLockManagerPluginPtr plugin, if (withResources) { VIR_DEBUG("Adding leases"); - for (i = 0; i < dom->def->nleases; i++) - if (virDomainLockManagerAddLease(lock, dom->def->leases[i]) < 0) + for (i = 0; i < def->nleases; i++) + if (virDomainLockManagerAddLease(lock, def->leases[i]) < 0) goto error; + } + if (withResources || metadataOnly) { VIR_DEBUG("Adding disks"); - for (i = 0; i < dom->def->ndisks; i++) { - virDomainDiskDefPtr disk = dom->def->disks[i]; + for (i = 0; i < def->ndisks; i++) { + virDomainDiskDefPtr disk = def->disks[i]; - if (virDomainLockManagerAddImage(lock, disk->src) < 0) + if (virDomainLockManagerAddImage(lock, disk->src, metadataOnly) < 0) goto error; } } + if (metadataOnly) { + for (i = 0; i < def->nmems; i++) { + virDomainMemoryDefPtr mem = def->mems[i]; + + if (virDomainLockManagerAddMemory(lock, mem) < 0) + goto error; + } + + if (def->os.loader && + virDomainLockManagerAddFile(lock, def->os.loader->nvram) < 0) + goto error; + + if (virDomainLockManagerAddFile(lock, def->os.kernel) < 0) + goto error; + + if (virDomainLockManagerAddFile(lock, def->os.initrd) < 0) + goto error; + + if (virDomainLockManagerAddFile(lock, def->os.dtb) < 0) + goto error; + + if (virDomainLockManagerAddFile(lock, def->os.slic_table) < 0) + goto error; + } + return lock; error: @@ -178,7 +265,7 @@ int virDomainLockProcessStart(virLockManagerPluginPtr plugin, VIR_DEBUG("plugin=%p dom=%p paused=%d fd=%p", plugin, dom, paused, fd); - if (!(lock = virDomainLockManagerNew(plugin, uri, dom, true, + if (!(lock = virDomainLockManagerNew(plugin, uri, dom, true, false, VIR_LOCK_MANAGER_NEW_STARTED))) return -1; @@ -203,7 +290,7 @@ int virDomainLockProcessPause(virLockManagerPluginPtr plugin, VIR_DEBUG("plugin=%p dom=%p state=%p", plugin, dom, state); - if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, true, 0))) + if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, true, false, 0))) return -1; ret = virLockManagerRelease(lock, state, 0); @@ -223,7 +310,7 @@ int virDomainLockProcessResume(virLockManagerPluginPtr plugin, VIR_DEBUG("plugin=%p dom=%p state=%s", plugin, dom, NULLSTR(state)); - if (!(lock = virDomainLockManagerNew(plugin, uri, dom, true, 0))) + if (!(lock = virDomainLockManagerNew(plugin, uri, dom, true, false, 0))) return -1; ret = virLockManagerAcquire(lock, state, 0, dom->def->onLockFailure, NULL); @@ -242,7 +329,7 @@ int virDomainLockProcessInquire(virLockManagerPluginPtr plugin, VIR_DEBUG("plugin=%p dom=%p state=%p", plugin, dom, state); - if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, true, 0))) + if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, true, false, 0))) return -1; ret = virLockManagerInquire(lock, state, 0); @@ -262,10 +349,10 @@ int virDomainLockImageAttach(virLockManagerPluginPtr plugin, VIR_DEBUG("plugin=%p dom=%p src=%p", plugin, dom, src); - if (!(lock = virDomainLockManagerNew(plugin, uri, dom, false, 0))) + if (!(lock = virDomainLockManagerNew(plugin, uri, dom, false, false, 0))) return -1; - if (virDomainLockManagerAddImage(lock, src) < 0) + if (virDomainLockManagerAddImage(lock, src, false) < 0) goto cleanup; if (virLockManagerAcquire(lock, NULL, 0, @@ -299,10 +386,10 @@ int virDomainLockImageDetach(virLockManagerPluginPtr plugin, VIR_DEBUG("plugin=%p dom=%p src=%p", plugin, dom, src); - if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, false, 0))) + if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, false, false, 0))) return -1; - if (virDomainLockManagerAddImage(lock, src) < 0) + if (virDomainLockManagerAddImage(lock, src, false) < 0) goto cleanup; if (virLockManagerRelease(lock, NULL, 0) < 0) @@ -336,7 +423,7 @@ int virDomainLockLeaseAttach(virLockManagerPluginPtr plugin, VIR_DEBUG("plugin=%p dom=%p lease=%p", plugin, dom, lease); - if (!(lock = virDomainLockManagerNew(plugin, uri, dom, false, 0))) + if (!(lock = virDomainLockManagerNew(plugin, uri, dom, false, false, 0))) return -1; if (virDomainLockManagerAddLease(lock, lease) < 0) @@ -364,7 +451,7 @@ int virDomainLockLeaseDetach(virLockManagerPluginPtr plugin, VIR_DEBUG("plugin=%p dom=%p lease=%p", plugin, dom, lease); - if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, false, 0))) + if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, false, false, 0))) return -1; if (virDomainLockManagerAddLease(lock, lease) < 0) @@ -380,3 +467,174 @@ int virDomainLockLeaseDetach(virLockManagerPluginPtr plugin, return ret; } + + +int +virDomainLockMetadataLock(virLockManagerPluginPtr plugin, + virDomainObjPtr dom) +{ + virLockManagerPtr lock; + const unsigned int flags = 0; + int ret = -1; + + VIR_DEBUG("plugin=%p dom=%p", plugin, dom); + + if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, false, true, 0))) + return -1; + + if (virLockManagerAcquire(lock, NULL, flags, + VIR_DOMAIN_LOCK_FAILURE_DEFAULT, NULL) < 0) + goto cleanup; + + ret = 0; + cleanup: + virLockManagerFree(lock); + return ret; +} + + +int +virDomainLockMetadataUnlock(virLockManagerPluginPtr plugin, + virDomainObjPtr dom) +{ + virLockManagerPtr lock; + const unsigned int flags = 0; + int ret = -1; + + VIR_DEBUG("plugin=%p dom=%p", plugin, dom); + + if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, false, true, 0))) + return -1; + + if (virLockManagerRelease(lock, NULL, flags) < 0) + goto cleanup; + + ret = 0; + cleanup: + virLockManagerFree(lock); + return ret; +} + + +int +virDomainLockMetadataImageLock(virLockManagerPluginPtr plugin, + virDomainObjPtr dom, + virStorageSourcePtr src) +{ + virLockManagerPtr lock; + int ret = -1; + + VIR_DEBUG("plugin=%p dom=%p src=%p", plugin, dom, src); + + if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, false, false, 0))) + return -1; + + if (virDomainLockManagerAddImage(lock, src, true) < 0) + goto cleanup; + + if (virLockManagerAcquire(lock, NULL, 0, + VIR_DOMAIN_LOCK_FAILURE_DEFAULT, NULL) < 0) + goto cleanup; + + ret = 0; + cleanup: + virLockManagerFree(lock); + return ret; +} + + +int +virDomainLockMetadataImageUnlock(virLockManagerPluginPtr plugin, + virDomainObjPtr dom, + virStorageSourcePtr src) +{ + virLockManagerPtr lock; + int ret = -1; + + VIR_DEBUG("plugin=%p dom=%p src=%p", plugin, dom, src); + + if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, false, false, 0))) + return -1; + + if (virDomainLockManagerAddImage(lock, src, true) < 0) + goto cleanup; + + if (virLockManagerRelease(lock, NULL, 0) < 0) + goto cleanup; + + ret = 0; + cleanup: + virLockManagerFree(lock); + return ret; +} + + +int +virDomainLockMetadataDiskLock(virLockManagerPluginPtr plugin, + virDomainObjPtr dom, + virDomainDiskDefPtr disk) +{ + return virDomainLockMetadataImageLock(plugin, dom, disk->src); +} + + +int +virDomainLockMetadataDiskUnlock(virLockManagerPluginPtr plugin, + virDomainObjPtr dom, + virDomainDiskDefPtr disk) +{ + return virDomainLockMetadataImageUnlock(plugin, dom, disk->src); +} + + +int +virDomainLockMetadataMemLock(virLockManagerPluginPtr plugin, + virDomainObjPtr dom, + virDomainMemoryDefPtr mem) +{ + virLockManagerPtr lock; + int ret = -1; + + VIR_DEBUG("plugin=%p dom=%p mem=%p", plugin, dom, mem); + + if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, false, false, 0))) + return -1; + + if (virDomainLockManagerAddMemory(lock, mem) < 0) + goto cleanup; + + if (virLockManagerAcquire(lock, NULL, 0, + VIR_DOMAIN_LOCK_FAILURE_DEFAULT, NULL) < 0) + goto cleanup; + + ret = 0; + cleanup: + virLockManagerFree(lock); + return ret; +} + + +int +virDomainLockMetadataMemUnlock(virLockManagerPluginPtr plugin, + virDomainObjPtr dom, + virDomainMemoryDefPtr mem) +{ + virLockManagerPtr lock; + int ret = -1; + + VIR_DEBUG("plugin=%p dom=%p mem=%p", plugin, dom, mem); + + if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, false, false, 0))) + return -1; + + if (virDomainLockManagerAddMemory(lock, mem) < 0) + goto cleanup; + + if (virLockManagerRelease(lock, NULL, 0) < 0) + goto cleanup; + + ret = 0; + cleanup: + virLockManagerFree(lock); + return ret; +} diff --git a/src/locking/domain_lock.h b/src/locking/domain_lock.h index fb4910230c..fbd3ee1d4e 100644 --- a/src/locking/domain_lock.h +++ b/src/locking/domain_lock.h @@ -66,4 +66,32 @@ int virDomainLockLeaseDetach(virLockManagerPluginPtr plugin, virDomainObjPtr dom, virDomainLeaseDefPtr lease); +int virDomainLockMetadataLock(virLockManagerPluginPtr plugin, + virDomainObjPtr dom); + +int virDomainLockMetadataUnlock(virLockManagerPluginPtr plugin, + virDomainObjPtr dom); + +int virDomainLockMetadataDiskLock(virLockManagerPluginPtr plugin, + virDomainObjPtr dom, + virDomainDiskDefPtr disk); +int virDomainLockMetadataDiskUnlock(virLockManagerPluginPtr plugin, + virDomainObjPtr dom, + virDomainDiskDefPtr disk); + +int virDomainLockMetadataImageLock(virLockManagerPluginPtr plugin, + virDomainObjPtr dom, + virStorageSourcePtr src); + +int virDomainLockMetadataImageUnlock(virLockManagerPluginPtr plugin, + virDomainObjPtr dom, + virStorageSourcePtr src); + +int virDomainLockMetadataMemLock(virLockManagerPluginPtr plugin, + virDomainObjPtr dom, + virDomainMemoryDefPtr mem); +int virDomainLockMetadataMemUnlock(virLockManagerPluginPtr plugin, + virDomainObjPtr dom, + virDomainMemoryDefPtr mem); + #endif /* __VIR_DOMAIN_LOCK_H__ */ -- 2.16.4
Fortunately, we have qemu wrappers so it's sufficient to put lock/unlock call only there. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_security.c | 107 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 107 insertions(+) diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c index af3be42854..527563947c 100644 --- a/src/qemu/qemu_security.c +++ b/src/qemu/qemu_security.c @@ -26,6 +26,7 @@ #include "qemu_domain.h" #include "qemu_security.h" #include "virlog.h" +#include "locking/domain_lock.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -39,6 +40,12 @@ qemuSecuritySetAllLabel(virQEMUDriverPtr driver, { int ret = -1; qemuDomainObjPrivatePtr priv = vm->privateData; + bool locked = false; + + if (virDomainLockMetadataLock(driver->lockManager, vm) < 0) + goto cleanup; + + locked = true; if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && virSecurityManagerTransactionStart(driver->securityManager) < 0) @@ -55,9 +62,17 @@ qemuSecuritySetAllLabel(virQEMUDriverPtr driver, vm->pid) < 0) goto cleanup; + locked = false; + + if (virDomainLockMetadataUnlock(driver->lockManager, vm) < 0) + goto cleanup; + ret = 0; cleanup: virSecurityManagerTransactionAbort(driver->securityManager); + if (locked && + virDomainLockMetadataUnlock(driver->lockManager, vm) < 0) + VIR_WARN("unable to release metadata lock"); return ret; } @@ -68,6 +83,10 @@ qemuSecurityRestoreAllLabel(virQEMUDriverPtr driver, bool migrated) { qemuDomainObjPrivatePtr priv = vm->privateData; + bool unlock = true; + + if (virDomainLockMetadataLock(driver->lockManager, vm) < 0) + unlock = false; /* In contrast to qemuSecuritySetAllLabel, do not use * secdriver transactions here. This function is called from @@ -79,6 +98,10 @@ qemuSecurityRestoreAllLabel(virQEMUDriverPtr driver, vm->def, migrated, priv->chardevStdioLogd); + + if (unlock && + virDomainLockMetadataUnlock(driver->lockManager, vm) < 0) + VIR_WARN("unable to release metadata lock"); } @@ -88,6 +111,12 @@ qemuSecuritySetDiskLabel(virQEMUDriverPtr driver, virDomainDiskDefPtr disk) { int ret = -1; + bool locked = false; + + if (virDomainLockMetadataDiskLock(driver->lockManager, vm, disk) < 0) + goto cleanup; + + locked = true; if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && virSecurityManagerTransactionStart(driver->securityManager) < 0) @@ -103,9 +132,17 @@ qemuSecuritySetDiskLabel(virQEMUDriverPtr driver, vm->pid) < 0) goto cleanup; + locked = false; + + if (virDomainLockMetadataDiskUnlock(driver->lockManager, vm, disk) < 0) + goto cleanup; + ret = 0; cleanup: virSecurityManagerTransactionAbort(driver->securityManager); + if (locked && + virDomainLockMetadataDiskUnlock(driver->lockManager, vm, disk) < 0) + VIR_WARN("unable to release disk metadata lock"); return ret; } @@ -116,6 +153,12 @@ qemuSecurityRestoreDiskLabel(virQEMUDriverPtr driver, virDomainDiskDefPtr disk) { int ret = -1; + bool locked = false; + + if (virDomainLockMetadataDiskLock(driver->lockManager, vm, disk) < 0) + goto cleanup; + + locked = true; if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && virSecurityManagerTransactionStart(driver->securityManager) < 0) @@ -131,9 +174,17 @@ qemuSecurityRestoreDiskLabel(virQEMUDriverPtr driver, vm->pid) < 0) goto cleanup; + locked = false; + + if (virDomainLockMetadataDiskUnlock(driver->lockManager, vm, disk) < 0) + goto cleanup; + ret = 0; cleanup: virSecurityManagerTransactionAbort(driver->securityManager); + if (locked && + virDomainLockMetadataDiskUnlock(driver->lockManager, vm, disk) < 0) + VIR_WARN("unable to release disk metadata lock"); return ret; } @@ -144,6 +195,12 @@ qemuSecuritySetImageLabel(virQEMUDriverPtr driver, virStorageSourcePtr src) { int ret = -1; + bool locked = false; + + if (virDomainLockMetadataImageLock(driver->lockManager, vm, src) < 0) + goto cleanup; + + locked = true; if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && virSecurityManagerTransactionStart(driver->securityManager) < 0) @@ -159,9 +216,17 @@ qemuSecuritySetImageLabel(virQEMUDriverPtr driver, vm->pid) < 0) goto cleanup; + locked = false; + + if (virDomainLockMetadataImageUnlock(driver->lockManager, vm, src) < 0) + goto cleanup; + ret = 0; cleanup: virSecurityManagerTransactionAbort(driver->securityManager); + if (locked && + virDomainLockMetadataImageUnlock(driver->lockManager, vm, src) < 0) + VIR_WARN("unable to release image metadata lock"); return ret; } @@ -172,6 +237,12 @@ qemuSecurityRestoreImageLabel(virQEMUDriverPtr driver, virStorageSourcePtr src) { int ret = -1; + bool locked = false; + + if (virDomainLockMetadataImageLock(driver->lockManager, vm, src) < 0) + goto cleanup; + + locked = true; if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && virSecurityManagerTransactionStart(driver->securityManager) < 0) @@ -187,9 +258,17 @@ qemuSecurityRestoreImageLabel(virQEMUDriverPtr driver, vm->pid) < 0) goto cleanup; + locked = false; + + if (virDomainLockMetadataImageUnlock(driver->lockManager, vm, src) < 0) + goto cleanup; + ret = 0; cleanup: virSecurityManagerTransactionAbort(driver->securityManager); + if (locked && + virDomainLockMetadataImageUnlock(driver->lockManager, vm, src) < 0) + VIR_WARN("unable to release image metadata lock"); return ret; } @@ -258,6 +337,12 @@ qemuSecuritySetMemoryLabel(virQEMUDriverPtr driver, virDomainMemoryDefPtr mem) { int ret = -1; + bool locked = false; + + if (virDomainLockMetadataMemLock(driver->lockManager, vm, mem) < 0) + goto cleanup; + + locked = true; if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && virSecurityManagerTransactionStart(driver->securityManager) < 0) @@ -273,9 +358,17 @@ qemuSecuritySetMemoryLabel(virQEMUDriverPtr driver, vm->pid) < 0) goto cleanup; + locked = false; + + if (virDomainLockMetadataMemUnlock(driver->lockManager, vm, mem) < 0) + goto cleanup; + ret = 0; cleanup: virSecurityManagerTransactionAbort(driver->securityManager); + if (locked && + virDomainLockMetadataMemUnlock(driver->lockManager, vm, mem) < 0) + VIR_WARN("unable to release memory metadata lock"); return ret; } @@ -286,6 +379,12 @@ qemuSecurityRestoreMemoryLabel(virQEMUDriverPtr driver, virDomainMemoryDefPtr mem) { int ret = -1; + bool locked = false; + + if (virDomainLockMetadataMemLock(driver->lockManager, vm, mem) < 0) + goto cleanup; + + locked = true; if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && virSecurityManagerTransactionStart(driver->securityManager) < 0) @@ -301,9 +400,17 @@ qemuSecurityRestoreMemoryLabel(virQEMUDriverPtr driver, vm->pid) < 0) goto cleanup; + locked = false; + + if (virDomainLockMetadataMemUnlock(driver->lockManager, vm, mem) < 0) + goto cleanup; + ret = 0; cleanup: virSecurityManagerTransactionAbort(driver->securityManager); + if (locked && + virDomainLockMetadataMemUnlock(driver->lockManager, vm, mem) < 0) + VIR_WARN("unable to release memory metadata lock"); return ret; } -- 2.16.4
participants (2)
-
Daniel P. Berrangé -
Michal Privoznik