Re: [PATCH v2 01/31] qemu: Make QMP probing process reusable

[Jiri Denemark <jdenemar@xxxxxxxxxx>]

On Thu, Nov 24, 2016 at 14:54:48 +0100, Pavel Hrdina wrote:
> On Mon, Nov 21, 2016 at 12:20:57AM +0100, Jiri Denemark wrote:
> > The code that runs a new QEMU process to be used for probing
> > capabilities is separated into four reusable functions so that any code
> > that wants to probe a QEMU process may just follow a few simple steps:
> > 
> >     cmd = virQEMUCapsInitQMPCommandNew(...);
> > 
> >     mon = virQEMUCapsInitQMPCommandRun(cmd, ...);
> > 
> >     /* talk to the running QEMU process using its QMP monitor */
> > 
> >     if (reprobeIsRequired) {
> >         virQEMUCapsInitQMPCommandAbort(cmd, ...);
> >         mon = virQEMUCapsInitQMPCommandRun(cmd, ...);
> > 
> >         /* talk to the running QEMU process again */
> >     }
> > 
> >     virQEMUCapsInitQMPCommandFree(cmd);
> > 
> > Signed-off-by: Jiri Denemark <jdenemar@xxxxxxxxxx>
> > ---
> >  src/qemu/qemu_capabilities.c | 259 +++++++++++++++++++++++++++++--------------
> >  1 file changed, 174 insertions(+), 85 deletions(-)
> > 
> > +static qemuMonitorPtr
> > +virQEMUCapsInitQMPCommandRun(virQEMUCapsInitQMPCommandPtr cmd,
> > +                             bool *qemuFailed)
> 
> That *bool* is not necessary, function virQEMUCapsInitQMPCommandRun can return
> simple *int* because qemuMonitorPtr is stored in *cmd*.  So this function can
> return -1 in case of fatal error.  In case return is 0, following code can
> easily depend on whether cmd->mon is set or not.

Indeed, although I think the function should rather return -1, 0, 1
instead of forcing the code to do magic based on cmd->mon.

> Otherwise this patch looks good, but I think that it would be better to send a
> new version with the suggested change.

Sure, since the patch is quite large and not exactly easy to read, the
following is the diff I will squash into v2.

Jirka

diff --git i/src/qemu/qemu_capabilities.c w/src/qemu/qemu_capabilities.c
index 1257bb0ff..d6182ccb7 100644
--- i/src/qemu/qemu_capabilities.c
+++ w/src/qemu/qemu_capabilities.c
@@ -4143,12 +4143,16 @@ virQEMUCapsInitQMPCommandNew(char *binary,
 }
 
 
-static qemuMonitorPtr
-virQEMUCapsInitQMPCommandRun(virQEMUCapsInitQMPCommandPtr cmd,
-                             bool *qemuFailed)
+/* Returns -1 on fatal error,
+ *          0 on success,
+ *          1 when probing QEMU failed
+ */
+static int
+virQEMUCapsInitQMPCommandRun(virQEMUCapsInitQMPCommandPtr cmd)
 {
     virDomainXMLOptionPtr xmlopt = NULL;
     int status = 0;
+    int ret = -1;
 
     VIR_DEBUG("Try to probe capabilities of '%s' via QMP", cmd->binary);
 
@@ -4203,15 +4207,17 @@ virQEMUCapsInitQMPCommandRun(virQEMUCapsInitQMPCommandPtr cmd,
 
     virObjectLock(cmd->mon);
 
+    ret = 0;
+
  cleanup:
     if (!cmd->mon)
         virQEMUCapsInitQMPCommandAbort(cmd);
     virObjectUnref(xmlopt);
 
-    return cmd->mon;
+    return ret;
 
  ignore:
-    *qemuFailed = true;
+    ret = 1;
     goto cleanup;
 }
 
@@ -4224,21 +4230,20 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps,
                    char **qmperr)
 {
     virQEMUCapsInitQMPCommandPtr cmd = NULL;
-    qemuMonitorPtr mon = NULL;
-    bool qemuFailed = false;
     int ret = -1;
+    int rc;
 
     if (!(cmd = virQEMUCapsInitQMPCommandNew(qemuCaps->binary, libDir,
                                              runUid, runGid, qmperr)))
         goto cleanup;
 
-    if (!(mon = virQEMUCapsInitQMPCommandRun(cmd, &qemuFailed))) {
-        if (qemuFailed)
+    if ((rc = virQEMUCapsInitQMPCommandRun(cmd)) != 0) {
+        if (rc == 1)
             ret = 0;
         goto cleanup;
     }
 
-    if (virQEMUCapsInitQMPMonitor(qemuCaps, mon) < 0)
+    if (virQEMUCapsInitQMPMonitor(qemuCaps, cmd->mon) < 0)
         goto cleanup;
 
     ret = 0;

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