Re: [libvirt] [PATCH 13/14] qemu_hotplug: Hotplug of reservations

12 Feb 2018

On Thu, Jan 18, 2018 at 17:04:45 +0100, Michal Privoznik wrote:
...
Surprisingly, nothing special is happening here. If we are the
first to use the managed helper then spawn it. If not, we're
almost done.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/qemu/qemu_hotplug.c | 97 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 97 insertions(+)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 6b245bd6a..ded666633 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -350,6 +350,84 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
 }
+static int
+qemuBuildPRDefInfoProps(virDomainObjPtr vm,
+                        virDomainDiskDefPtr disk,
+                        virJSONValuePtr *prmgrProps,
+                        const char **prAlias)
+{
+    qemuDomainObjPrivatePtr priv = vm->privateData;
+    qemuDomainStorageSourcePrivatePtr srcPriv;
+    qemuDomainDiskPRObjectPtr tmp;
+    virJSONValuePtr props = NULL;
+    int ret = -1;
This function used in conjucntion with the JSON->commandline formatter
should be used instead of qemuBuildMasterPRCommandLineHelper so that we
don't have multiple implementations which need to be kept in sync ...
...
+
+    srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src);
+
+    *prmgrProps = NULL;
+
+    if (!priv->prHelpers ||
+        !srcPriv->prAlias ||
+        !(tmp = virHashLookup(priv->prHelpers, srcPriv->prAlias)))
+        return 0;
+
+    if (qemuDomainGetPRUsageCount(vm->def, srcPriv->prAlias) != 0) {
+        /* We're not the first ones to use pr-manager with that
+         * alias.  Return early and leave @prmgrProps NULL so
+         * that caller knows this. */
+        return 0;
+    }
Obviously you'll still need a wrapper for this logic ...
...
+
+    if (virJSONValueObjectCreate(&props,
+                                 "s:path", tmp->path,
+                                 NULL) < 0)
... but we should not have two different implementations of this.
...
+        goto cleanup;
+
+    if (qemuProcessSetupPRDaemon(vm, tmp, srcPriv->prAlias) < 0)
+        goto cleanup;
+
+    *prAlias = srcPriv->prAlias;
+    *prmgrProps = props;
+    props = NULL;
+    ret = 0;
+ cleanup:
+    virJSONValueFree(props);
+    return ret;
+}
+
+
+static void
+qemuDestroyPRDefObject(virDomainObjPtr vm,
+                       virDomainDiskDefPtr disk)
+{
+    qemuDomainObjPrivatePtr priv = vm->privateData;
+    qemuDomainStorageSourcePrivatePtr srcPriv;
+    qemuDomainDiskPRObjectPtr tmp;
+
+    srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src);
+
+    if (!priv->prHelpers ||
+        !srcPriv->prAlias ||
+        !(tmp = virHashLookup(priv->prHelpers, srcPriv->prAlias)))
+        return;
+
+    if (qemuDomainGetPRUsageCount(vm->def, srcPriv->prAlias) > 1) {
+        /* The function might return one or even zero if the
+         * pr-manager is unused depending whether the disk was
+         * added to domain def already or not. Anyway, if the
+         * return value is greater than one we are certain that
+         * there's another disk using it so return early. */
Shouldn't we use a different approach rather than comments like this?
...
+        return;
+    }
+
+    /* This also kills the pr-manager daemon. See
+     * qemuDomainDiskPRObjectHashFree. */
+    virHashRemoveEntry(priv->prHelpers, srcPriv->prAlias);
+
+    VIR_FREE(srcPriv->prAlias);
+}
+
+
 /**
  * qemuDomainAttachDiskGeneric:
  *

    

Re: [libvirt] [PATCH 13/14] qemu_hotplug: Hotplug of reservations

Peter Krempa