qemu: hotplug: Refactor for -blockdev support (blockdev-add saga)

[libvirt] [PATCH 07/10] qemu: hotplug: Simplify removal of managed PR infrastructure on unplug

Posted by Peter Krempa 7 years, 6 months ago

Extract the (possible) removal of the PR backend and daemon into a
separate helper which enters monitor on it's own. This simplifies the
code and allows reuse of this function in the future e.g. for blockjobs
where removing a image with PR may result into PR not being necessary.

Since the PR is not used often the overhead of entering monitor again
should be negligible.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 src/qemu/qemu_hotplug.c | 49 ++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 38 insertions(+), 11 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 3ee74c8e40..57ab753974 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -350,6 +350,41 @@ qemuDomainDiskAttachManagedPR(virDomainObjPtr vm,
 }


+/**
+ * qemuHotplugRemoveManagedPR:
+ * @driver: QEMU driver object
+ * @vm: domain object
+ * @asyncJob: asynchronous job identifier
+ *
+ * Removes the managed PR object from @vm if the configuration does not require
+ * it any more.
+ */
+static int
+qemuHotplugRemoveManagedPR(virQEMUDriverPtr driver,
+                           virDomainObjPtr vm,
+                           qemuDomainAsyncJob asyncJob)
+{
+    qemuDomainObjPrivatePtr priv = vm->privateData;
+    virErrorPtr orig_err;
+    virErrorPreserveLast(&orig_err);
+
+    if (!priv->prDaemonRunning ||
+        virDomainDefHasManagedPR(vm->def))
+        return 0;
+
+    if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
+        return -1;
+    ignore_value(qemuMonitorDelObject(priv->mon, qemuDomainGetManagedPRAlias()));
+    if (qemuDomainObjExitMonitor(driver, vm) < 0)
+        return -1;
+
+    qemuProcessKillManagedPRDaemon(vm);
+    virErrorRestore(&orig_err);
+
+    return 0;
+}
+
+
 struct _qemuHotplugDiskSourceData {
     qemuBlockStorageSourceAttachDataPtr *backends;
     size_t nbackends;
@@ -3948,8 +3983,6 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
     virObjectEventPtr event;
     size_t i;
     qemuDomainObjPrivatePtr priv = vm->privateData;
-    bool prManaged = priv->prDaemonRunning;
-    bool prUsed = false;
     int ret = -1;

     VIR_DEBUG("Removing disk %s from domain %p %s",
@@ -3965,16 +3998,10 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
         }
     }

-    /* check if the last disk with managed PR was just removed */
-    prUsed = virDomainDefHasManagedPR(vm->def);
-
     qemuDomainObjEnterMonitor(driver, vm);

     qemuHotplugDiskSourceRemove(priv->mon, diskbackend);

-    if (prManaged && !prUsed)
-        ignore_value(qemuMonitorDelObject(priv->mon, qemuDomainGetManagedPRAlias()));
-
     if (qemuDomainObjExitMonitor(driver, vm) < 0)
         goto cleanup;

@@ -3983,9 +4010,6 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
     event = virDomainEventDeviceRemovedNewFromObj(vm, disk->info.alias);
     virObjectEventStateQueue(driver->domainEventState, event);

-    if (prManaged && !prUsed)
-        qemuProcessKillManagedPRDaemon(vm);
-
     qemuDomainReleaseDeviceAddress(vm, &disk->info, virDomainDiskGetSource(disk));

     /* tear down disk security access */
@@ -3996,6 +4020,9 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
     ignore_value(qemuRemoveSharedDevice(driver, &dev, vm->def->name));
     virDomainUSBAddressRelease(priv->usbaddrs, &disk->info);

+    if (qemuHotplugRemoveManagedPR(driver, vm, QEMU_ASYNC_JOB_NONE) < 0)
+        goto cleanup;
+
     ret = 0;

  cleanup:
-- 
2.16.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 07/10] qemu: hotplug: Simplify removal of managed PR infrastructure on unplug

Posted by Ján Tomko 7 years, 6 months ago

On Tue, Jul 17, 2018 at 02:14:27PM +0200, Peter Krempa wrote:
>Extract the (possible) removal of the PR backend and daemon into a
>separate helper which enters monitor on it's own. This simplifies the

its

>code and allows reuse of this function in the future e.g. for blockjobs
>where removing a image with PR may result into PR not being necessary.
>
>Since the PR is not used often the overhead of entering monitor again
>should be negligible.
>
>Signed-off-by: Peter Krempa <pkrempa@redhat.com>
>---
> src/qemu/qemu_hotplug.c | 49 ++++++++++++++++++++++++++++++++++++++-----------
> 1 file changed, 38 insertions(+), 11 deletions(-)
>

Reviewed-by: Ján Tomko <jtomko@redhat.com>

Jano
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 07/10] qemu: hotplug: Simplify removal of managed PR infrastructure on unplug

Posted by John Ferlan 7 years, 6 months ago


On 07/17/2018 08:14 AM, Peter Krempa wrote:
> Extract the (possible) removal of the PR backend and daemon into a
> separate helper which enters monitor on it's own. This simplifies the
> code and allows reuse of this function in the future e.g. for blockjobs
> where removing a image with PR may result into PR not being necessary.
> 
> Since the PR is not used often the overhead of entering monitor again
> should be negligible.
> 
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
>  src/qemu/qemu_hotplug.c | 49 ++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 38 insertions(+), 11 deletions(-)
> 
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 3ee74c8e40..57ab753974 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -350,6 +350,41 @@ qemuDomainDiskAttachManagedPR(virDomainObjPtr vm,
>  }
> 
> 
> +/**
> + * qemuHotplugRemoveManagedPR:
> + * @driver: QEMU driver object
> + * @vm: domain object
> + * @asyncJob: asynchronous job identifier
> + *
> + * Removes the managed PR object from @vm if the configuration does not require
> + * it any more.
> + */
> +static int
> +qemuHotplugRemoveManagedPR(virQEMUDriverPtr driver,
> +                           virDomainObjPtr vm,
> +                           qemuDomainAsyncJob asyncJob)
> +{
> +    qemuDomainObjPrivatePtr priv = vm->privateData;
> +    virErrorPtr orig_err;
> +    virErrorPreserveLast(&orig_err);

Coverity points out that orig_err is leaked at each subsequent return
before the virErrorRestore below

John

> +
> +    if (!priv->prDaemonRunning ||
> +        virDomainDefHasManagedPR(vm->def))
> +        return 0;
> +
> +    if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
> +        return -1;
> +    ignore_value(qemuMonitorDelObject(priv->mon, qemuDomainGetManagedPRAlias()));
> +    if (qemuDomainObjExitMonitor(driver, vm) < 0)
> +        return -1;
> +
> +    qemuProcessKillManagedPRDaemon(vm);
> +    virErrorRestore(&orig_err);
> +
> +    return 0;
> +}
> +
> +

[...]

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list