Libvirt only manages one PR daemon. This means that we don't need to
pass the 'disk' object and also rename the functions dealing with this
so that it's obvious we only deal with the managed PR daemon.

Signed-off-by: Peter Krempa <pkrempa@redhat st.com>
---
 src/qemu/qemu_hotplug.c |  6 +++---
 src/qemu/qemu_process.c | 37 ++++++++++++++++---------------------
 src/qemu/qemu_process.h |  5 ++---
 3 files changed, 21 insertions(+), 27 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 77d37e5ef6..3a26876c10 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -377,7 +377,7 @@ qemuDomainMaybeStartPRDaemon(virDomainObjPtr vm,

     /* @disk requires qemu-pr-helper but none is running.
      * Start it now. */
-    if (qemuProcessStartPRDaemon(vm, disk) < 0)
+    if (qemuProcessStartManagedPRDaemon(vm) < 0)
         return -1;

     return 1;
@@ -567,7 +567,7 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver,
     qemuDomainDelDiskSrcTLSObject(driver, vm, disk->src);
     ignore_value(qemuHotplugPrepareDiskAccess(driver, vm, disk, NULL, true));
     if (prdStarted)
-        qemuProcessKillPRDaemon(vm);
+        qemuProcessKillManagedPRDaemon(vm);
     goto cleanup;
 }

@@ -3963,7 +3963,7 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
     }

     if (stopPRDaemon)
-        qemuProcessKillPRDaemon(vm);
+        qemuProcessKillManagedPRDaemon(vm);

     qemuDomainReleaseDeviceAddress(vm, &disk->info, src);

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 42b91b39ac..af29bcc59e 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -2566,7 +2566,7 @@ qemuProcessBuildPRHelperPidfilePath(virDomainObjPtr vm)


 void
-qemuProcessKillPRDaemon(virDomainObjPtr vm)
+qemuProcessKillManagedPRDaemon(virDomainObjPtr vm)
 {
     qemuDomainObjPrivatePtr priv = vm->privateData;
     virErrorPtr orig_err;
@@ -2624,8 +2624,7 @@ qemuProcessStartPRDaemonHook(void *opaque)


 int
-qemuProcessStartPRDaemon(virDomainObjPtr vm,
-                         const virDomainDiskDef *disk)
+qemuProcessStartManagedPRDaemon(virDomainObjPtr vm)
 {
     qemuDomainObjPrivatePtr priv = vm->privateData;
     virQEMUDriverPtr driver = priv->driver;
@@ -2640,10 +2639,6 @@ qemuProcessStartPRDaemon(virDomainObjPtr vm,
     const unsigned long long timeout = 500000; /* ms */
     int ret = -1;

-    if (!virStoragePRDefIsManaged(disk->src->pr) ||
-        priv->prDaemonRunning)
-        return 0;
-
     cfg = virQEMUDriverGetConfig(driver);

     if (!virFileIsExecutable(cfg->prHelperName)) {
@@ -2734,7 +2729,7 @@ qemuProcessStartPRDaemon(virDomainObjPtr vm,
         goto cleanup;

     priv->prDaemonRunning = true;
-    ret = 1;
+    ret = 0;
  cleanup:
     if (ret < 0) {
         virCommandAbort(cmd);
@@ -2754,22 +2749,22 @@ qemuProcessStartPRDaemon(virDomainObjPtr vm,


 static int
-qemuProcessMaybeStartPRDaemon(virDomainObjPtr vm)
+qemuProcessMaybeStartManagedPRDaemon(virDomainObjPtr vm)
 {
+    bool hasManaged = false;
     size_t i;
-    int rv;

     for (i = 0; i < vm->def->ndisks; i++) {
-        const virDomainDiskDef *disk = vm->def->disks[i];
-
-        if ((rv = qemuProcessStartPRDaemon(vm, disk)) < 0)
-            return -1;
-
-        if (rv > 0)
-            return 1;
+        if (virStoragePRDefIsManaged(vm->def->disks[i]->src->pr)) {
+            hasManaged = true;
+            break;
+        }
     }

-    return 0;
+    if (!hasManaged)
+        return 0;
+
+    return qemuProcessStartManagedPRDaemon(vm);
 }


@@ -6289,8 +6284,8 @@ qemuProcessLaunch(virConnectPtr conn,
     if (qemuProcessResctrlCreate(driver, vm) < 0)
         goto cleanup;

-    VIR_DEBUG("Setting up PR daemon");
-    if (qemuProcessMaybeStartPRDaemon(vm) < 0)
+    VIR_DEBUG("Setting up managed PR daemon");
+    if (qemuProcessMaybeStartManagedPRDaemon(vm) < 0)
         goto cleanup;

     VIR_DEBUG("Setting domain security labels");
@@ -6821,7 +6816,7 @@ void qemuProcessStop(virQEMUDriverPtr driver,
     qemuDomainMasterKeyRemove(priv);

     /* Do this before we delete the tree and remove pidfile. */
-    qemuProcessKillPRDaemon(vm);
+    qemuProcessKillManagedPRDaemon(vm);

     virFileDeleteTree(priv->libDir);
     virFileDeleteTree(priv->channelTargetDir);
diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h
index eda6695415..a0e34b1c85 100644
--- a/src/qemu/qemu_process.h
+++ b/src/qemu/qemu_process.h
@@ -205,9 +205,8 @@ int qemuProcessRefreshDisks(virQEMUDriverPtr driver,
                             virDomainObjPtr vm,
                             qemuDomainAsyncJob asyncJob);

-int qemuProcessStartPRDaemon(virDomainObjPtr vm,
-                             const virDomainDiskDef *disk);
+int qemuProcessStartManagedPRDaemon(virDomainObjPtr vm);

-void qemuProcessKillPRDaemon(virDomainObjPtr vm);
+void qemuProcessKillManagedPRDaemon(virDomainObjPtr vm);

 #endif /* __QEMU_PROCESS_H__ */
-- 
2.16.2

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

On 05/14/2018 06:45 AM, Peter Krempa wrote:
> Libvirt only manages one PR daemon. This means that we don't need to
> pass the 'disk' object and also rename the functions dealing with this
> so that it's obvious we only deal with the managed PR daemon.
> 
> Signed-off-by: Peter Krempa <pkrempa@redhat st.com>
                                             ^^^^

Something strange happened here.


> ---
>  src/qemu/qemu_hotplug.c |  6 +++---
>  src/qemu/qemu_process.c | 37 ++++++++++++++++---------------------
>  src/qemu/qemu_process.h |  5 ++---
>  3 files changed, 21 insertions(+), 27 deletions(-)
> 

[...]

>  int
> -qemuProcessStartPRDaemon(virDomainObjPtr vm,
> -                         const virDomainDiskDef *disk)
> +qemuProcessStartManagedPRDaemon(virDomainObjPtr vm)
>  {
>      qemuDomainObjPrivatePtr priv = vm->privateData;
>      virQEMUDriverPtr driver = priv->driver;
> @@ -2640,10 +2639,6 @@ qemuProcessStartPRDaemon(virDomainObjPtr vm,
>      const unsigned long long timeout = 500000; /* ms */
>      int ret = -1;
> 
> -    if (!virStoragePRDefIsManaged(disk->src->pr) ||
> -        priv->prDaemonRunning)
> -        return 0;
> -
>      cfg = virQEMUDriverGetConfig(driver);
> 
>      if (!virFileIsExecutable(cfg->prHelperName)) {
> @@ -2734,7 +2729,7 @@ qemuProcessStartPRDaemon(virDomainObjPtr vm,
>          goto cleanup;
> 
>      priv->prDaemonRunning = true;
> -    ret = 1;
> +    ret = 0;

Unrelated, but since callers only care about < 0, no big deal...   I'm
assuming this is a holder from some other path which used the function
for a return value (e.g. like qemuDomainMaybeStartPRDaemon) at some
point during development.


No big deal - I don't expect this to be extracted, but was just paying
attention ;-)

John


>   cleanup:
>      if (ret < 0) {
>          virCommandAbort(cmd);
> @@ -2754,22 +2749,22 @@ qemuProcessStartPRDaemon(virDomainObjPtr vm,
> 
> 

[...]

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

On Mon, May 14, 2018 at 16:33:58 -0400, John Ferlan wrote:
> 
> 
> On 05/14/2018 06:45 AM, Peter Krempa wrote:
> > Libvirt only manages one PR daemon. This means that we don't need to
> > pass the 'disk' object and also rename the functions dealing with this
> > so that it's obvious we only deal with the managed PR daemon.
> > 
> > Signed-off-by: Peter Krempa <pkrempa@redhat st.com>
>                                              ^^^^
> 
> Something strange happened here.

Yeah, I've messed up while editing the commit message somehow. It also
broke sending of the patches later on.

> 
> 
> > ---
> >  src/qemu/qemu_hotplug.c |  6 +++---
> >  src/qemu/qemu_process.c | 37 ++++++++++++++++---------------------
> >  src/qemu/qemu_process.h |  5 ++---
> >  3 files changed, 21 insertions(+), 27 deletions(-)
> > 
> 
> [...]
> 
> >  int
> > -qemuProcessStartPRDaemon(virDomainObjPtr vm,
> > -                         const virDomainDiskDef *disk)
> > +qemuProcessStartManagedPRDaemon(virDomainObjPtr vm)
> >  {
> >      qemuDomainObjPrivatePtr priv = vm->privateData;
> >      virQEMUDriverPtr driver = priv->driver;
> > @@ -2640,10 +2639,6 @@ qemuProcessStartPRDaemon(virDomainObjPtr vm,
> >      const unsigned long long timeout = 500000; /* ms */
> >      int ret = -1;
> > 
> > -    if (!virStoragePRDefIsManaged(disk->src->pr) ||
> > -        priv->prDaemonRunning)
> > -        return 0;
> > -
> >      cfg = virQEMUDriverGetConfig(driver);
> > 
> >      if (!virFileIsExecutable(cfg->prHelperName)) {
> > @@ -2734,7 +2729,7 @@ qemuProcessStartPRDaemon(virDomainObjPtr vm,
> >          goto cleanup;
> > 
> >      priv->prDaemonRunning = true;
> > -    ret = 1;
> > +    ret = 0;
> 
> Unrelated, but since callers only care about < 0, no big deal...   I'm
> assuming this is a holder from some other path which used the function
> for a return value (e.g. like qemuDomainMaybeStartPRDaemon) at some
> point during development.
> 
> 
> No big deal - I don't expect this to be extracted, but was just paying
> attention ;-)

The code path was actually exactly used in the case when
qemuDomainMaybeStartPRDaemon called into this function. Since the logic
determining whether it was started was extracted into that function (in
trimmed context) it now returns the correct thing there which is
expected and qemuProcessStartManagedPRDaemon does not have to do this
any more.

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