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 ca4a192a4a..720ae12301 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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 08/14/2018 07:19 AM, Michal Privoznik wrote:
> 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(-)
>
There seems to be more than just what's described going on in this patch
which could be split out....
1. Use @def in virDomainLockManagerNew
2. Add @metadataonly to virDomainLockManagerNew and
virDomainLockManagerAddImage
3. Introduce virDomainLockManagerAddMemory and
virDomainLockManagerAddFile along with changes to
virDomainLockManagerNew along with perhaps a few words why those files
were chosen and what decisions led to their selection. If someone adds
something in the future how would they know to add them to the list?
4. Various virDomainLockMetadata* API's.
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index ca4a192a4a..720ae12301 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;
Why not flip flop the order of new helpers and just call/return
virDomainLockManagerAddFile directly with 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;
And then just return 0 here. or just call it here since it returns 0 if
@!file anyway.
> +
> + 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) {
So this would seemingly answer my question long ago - one could get one
or both locks (@0 w/ length==1 and @1 w/ length==1)
> 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;
We never change this from 0 to something else (not even the next
patch)... Not that it matters, just noting it considering that other new
API's just pass 0 and not flags which is only ever set to 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)
Strange to see a longer second line, although I understand seeing NULL
along leads to why not just put it on the previous line type review comment.
FWIW: Seeing how @action is set here led me down the path of seeing how
it's used.
I wonder if perhaps that could be used in the lockd/sanlock code in
order to determine how/whether to fail rather than just returning 0 in
sanlock when METADATA isn't supported. If it really matters that is.
It's not 100% clear to me what/how the expected action usage should be.
> + goto cleanup;
> +
> + ret = 0;
> + cleanup:
> + virLockManagerFree(lock);
> + return ret;
> +}
> +
> +
> +int
> +virDomainLockMetadataUnlock(virLockManagerPluginPtr plugin,
> + virDomainObjPtr dom)
> +{
> + virLockManagerPtr lock;
> + const unsigned int flags = 0;
Similar @flags commentary.
> + int ret = -1;
> +
> + VIR_DEBUG("plugin=%p dom=%p", plugin, dom);
> +
> + if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, false, true, 0)))
Since MetadataLock/Unlock are the only ones passing true for the
@metadataOnly bool in virDomainLockManagerNew, I'm "conflicted" over
whether there should a helper for both in lieu of the bool, but then the
bool does the trick and it's following @withResources which was there
since inception, so I guess it's fine albeit odd w/r/t a *New API doing
more than just allocating memory or setting defaults.
Not an issue, but an observation, in case you were wondering. Trying to
learn a bit about this code while doing this review...
John
> + 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__ */
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 08/17/2018 07:58 PM, John Ferlan wrote:
>
>
> On 08/14/2018 07:19 AM, Michal Privoznik wrote:
>> 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(-)
>>
>
> There seems to be more than just what's described going on in this patch
> which could be split out....
>
> 1. Use @def in virDomainLockManagerNew
>
> 2. Add @metadataonly to virDomainLockManagerNew and
> virDomainLockManagerAddImage
>
> 3. Introduce virDomainLockManagerAddMemory and
> virDomainLockManagerAddFile along with changes to
> virDomainLockManagerNew along with perhaps a few words why those files
> were chosen and what decisions led to their selection. If someone adds
> something in the future how would they know to add them to the list?
>
> 4. Various virDomainLockMetadata* API's.
>
D'oh! Okay, I'll split this.
>
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index ca4a192a4a..720ae12301 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;
>
> Why not flip flop the order of new helpers and just call/return
> virDomainLockManagerAddFile directly with 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;
>
> And then just return 0 here. or just call it here since it returns 0 if
> @!file anyway.
Well, I think it's just a matter of preference though. My preference was
to have this future extensible. I mean, if we ever introduce new model
that we need to lock, this switch() would just get its path and the rest
is already handled. Whereas if I'd move AddResource() call right into
the switch() the call would be duplicated then.
>
>> +
>> + 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) {
>
> So this would seemingly answer my question long ago - one could get one
> or both locks (@0 w/ length==1 and @1 w/ length==1)
Yes. It was discussed here:
https://www.redhat.com/archives/libvir-list/2018-July/msg01880.html
>
>> 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;
>
> We never change this from 0 to something else (not even the next
> patch)... Not that it matters, just noting it considering that other new
> API's just pass 0 and not flags which is only ever set to 0.
I prefer avoiding magical constants. For example:
func(x, NULL, 0, 0);
vs
void *resources = NULL;
size_t nresources = 0;
uint flags = 0;
func(x, resources, nresources, flags);
>
>> + 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)
>
> Strange to see a longer second line, although I understand seeing NULL
> along leads to why not just put it on the previous line type review comment.
>
> FWIW: Seeing how @action is set here led me down the path of seeing how
> it's used.
>
> I wonder if perhaps that could be used in the lockd/sanlock code in
> order to determine how/whether to fail rather than just returning 0 in
> sanlock when METADATA isn't supported. If it really matters that is.
> It's not 100% clear to me what/how the expected action usage should be.
>
>> + goto cleanup;
>> +
>> + ret = 0;
>> + cleanup:
>> + virLockManagerFree(lock);
>> + return ret;
>> +}
>> +
>> +
>> +int
>> +virDomainLockMetadataUnlock(virLockManagerPluginPtr plugin,
>> + virDomainObjPtr dom)
>> +{
>> + virLockManagerPtr lock;
>> + const unsigned int flags = 0;
>
> Similar @flags commentary.
>
>> + int ret = -1;
>> +
>> + VIR_DEBUG("plugin=%p dom=%p", plugin, dom);
>> +
>> + if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, false, true, 0)))
>
> Since MetadataLock/Unlock are the only ones passing true for the
> @metadataOnly bool in virDomainLockManagerNew, I'm "conflicted" over
> whether there should a helper for both in lieu of the bool, but then the
> bool does the trick and it's following @withResources which was there
> since inception, so I guess it's fine albeit odd w/r/t a *New API doing
> more than just allocating memory or setting defaults.
>
> Not an issue, but an observation, in case you were wondering. Trying to
> learn a bit about this code while doing this review...
>
> John
>
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Aug 14, 2018 at 01:19:42PM +0200, Michal Privoznik wrote:
> 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).
I'm not sure I understand the rationale behind saying we only care
about resources on network filesystems.
If I have 2 locally running guests, and both have a serial port
backed by a physical serial port, eg
<serial type="dev">
<source path="/dev/ttyS0"/>
<target port="1"/>
</serial>
we *do* care about locking /dev/ttyS0, as libvirtd isn't doing
mutual exclusion checks anywhere else for the /dev/ttyS0 device
node.
In general I think we need to lock every single file resource
that is labelled for a guest, regardless of whether its local
or remote.
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 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Aug 20, 2018 at 04:07:28PM +0100, Daniel P. Berrangé wrote: > On Tue, Aug 14, 2018 at 01:19:42PM +0200, Michal Privoznik wrote: > > 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). > > I'm not sure I understand the rationale behind saying we only care > about resources on network filesystems. > > If I have 2 locally running guests, and both have a serial port > backed by a physical serial port, eg > > <serial type="dev"> > <source path="/dev/ttyS0"/> > <target port="1"/> > </serial> > > we *do* care about locking /dev/ttyS0, as libvirtd isn't doing > mutual exclusion checks anywhere else for the /dev/ttyS0 device > node. > > In general I think we need to lock every single file resource > that is labelled for a guest, regardless of whether its local > or remote. In the next patch I propose integration into the security manager that would avoid the need to touch this domain lock abstraction at all. 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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 08/20/2018 05:07 PM, Daniel P. Berrangé wrote: > On Tue, Aug 14, 2018 at 01:19:42PM +0200, Michal Privoznik wrote: >> 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). > > I'm not sure I understand the rationale behind saying we only care > about resources on network filesystems. > > If I have 2 locally running guests, and both have a serial port > backed by a physical serial port, eg > > <serial type="dev"> > <source path="/dev/ttyS0"/> > <target port="1"/> > </serial> > > we *do* care about locking /dev/ttyS0, as libvirtd isn't doing > mutual exclusion checks anywhere else for the /dev/ttyS0 device > node. Ah you mean that the system wide daemon and session daemon could clash when relabeling /dev/ttyS0? Well, we don't do relabeling for session daemons and running two system daemons is not supported (you're gonna hit more serious problems when trying that anyway). > > In general I think we need to lock every single file resource > that is labelled for a guest, regardless of whether its local > or remote. Well this might be feasible iff locking would be done from security driver. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Aug 20, 2018 at 07:13:46PM +0200, Michal Prívozník wrote: > On 08/20/2018 05:07 PM, Daniel P. Berrangé wrote: > > On Tue, Aug 14, 2018 at 01:19:42PM +0200, Michal Privoznik wrote: > >> 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). > > > > I'm not sure I understand the rationale behind saying we only care > > about resources on network filesystems. > > > > If I have 2 locally running guests, and both have a serial port > > backed by a physical serial port, eg > > > > <serial type="dev"> > > <source path="/dev/ttyS0"/> > > <target port="1"/> > > </serial> > > > > we *do* care about locking /dev/ttyS0, as libvirtd isn't doing > > mutual exclusion checks anywhere else for the /dev/ttyS0 device > > node. > > Ah you mean that the system wide daemon and session daemon could clash > when relabeling /dev/ttyS0? Well, we don't do relabeling for session > daemons and running two system daemons is not supported (you're gonna > hit more serious problems when trying that anyway). We certainly *do* have relabelling for session daemons, for SELinux: $ ls -alZ ~/VirtualMachines/demo.img -rw-r--r--. 1 berrange berrange unconfined_u:object_r:virt_home_t:s0 1073741824 Aug 21 09:25 /home/berrange/VirtualMachines/demo.img $ virsh uri qemu:///session $ virsh start QEMUGuest1 Domain QEMUGuest1 started $ ls -alZ ~/VirtualMachines/demo.img -rw-r--r--. 1 berrange berrange unconfined_u:object_r:svirt_image_t:s0:c310,c831 1073741824 Aug 21 09:25 /home/berrange/VirtualMachines/demo.img but I was more thinking about the future where we have the libvirt shim concept that allows starting & managing of QEMU processes independantly, and libvirtd is merely there for aggregated data reporting. In that case you'd have many instances of the security driver operating in parallel. 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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2024 Red Hat, Inc.