[libvirt] [PATCH v3 4/6] qemu_process: Use common processes mgmt funcs for all QMP query types

[Jiri Denemark]

On Thu, Sep 27, 2018 at 16:26:43 -0500, Chris Venteicher wrote:
> Generalized QEMU process management functions supporting all forms of
> QMP queries including but no limited to capabilities queries.
> 
> QEMU process instances can be maintained and used for multiple different QMP
> queries, of the same or different types, as required to complete a
> specific libvirt task.
> 
> Support concurrent QEMU process instances for QMP queries,
> using the same or different qemu binaries.
> 
> All process mgmt functions are removed from qemu_capabilities and
> re-implemented in qemu_process in a generic, non capabilities specific,
> manner consistent with existing qemu_process mgmt functions.
> 
> Concept of qmp_query domain is used to contain process state through the
> an entire process lifecycle similar to how a VM domains contain process
> state in existing process functions.
> 
> Monitor callbacks were not used in original process management code for
> capabilities and are eliminated in new generalized process code for QMP
> queries.
> 
> QMP exchange to exit capabilities negotiation mode and enter command mode
> is the sole responsibility of the QMP query process code.

Heh, a lot is happening here. Please, separate logical changes rather
than squashing them all in one patch.

I didn't succeed in following what all the changes are doing, but...

...
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 29b0ba1590..c8a34fc37e 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -8068,3 +8068,401 @@ qemuProcessReconnectAll(virQEMUDriverPtr driver)
>      struct qemuProcessReconnectData data = {.driver = driver};
>      virDomainObjListForEach(driver->domains, qemuProcessReconnectHelper, &data);
>  }
> +
> +
> +static void *
> +qmpDomainObjPrivateAlloc(void *opaque)
> +{
> +    qmpDomainObjPrivatePtr priv;
> +
> +    if (VIR_ALLOC(priv) < 0)
> +        return NULL;
> +
> +    VIR_DEBUG("priv=%p opaque=%p", priv, opaque);
> +
> +    return priv;
> +}

... this looks very suspicious. The code does not belong to
qemu_process.c.

> +
> +
> +static void
> +qmpDomainObjPrivateFree(void *data)
> +{
> +    qmpDomainObjPrivatePtr priv = data;
> +
> +    VIR_DEBUG("priv=%p, priv->mon=%p", priv, (priv ? priv->mon : NULL));
> +
> +    if (!priv)
> +        return;
> +
> +    /* This should never be non-NULL if we get here, but just in case... */
> +    if (priv->mon) {
> +        VIR_ERROR(_("Unexpected QEMU monitor still active during domain deletion"));
> +        qemuMonitorClose(priv->mon);
> +        priv->mon = NULL;
> +    }
> +
> +    VIR_FREE(priv->libDir);
> +    VIR_FREE(priv->monpath);
> +    VIR_FREE(priv->monarg);
> +    VIR_FREE(priv->pidfile);
> +    VIR_FREE(priv->qmperr);
> +    VIR_FREE(priv);
> +}
> +
> +/**
> + * qmpDomainObjNew:
> + * @binary: Qemu binary
> + * @forceTCG: Force TCG mode if true
> + * @libDir: Directory for process and connection artifacts
> + * @runUid: UserId for Qemu Process
> + * @runGid: GroupId for Qemu Process
> + *
> + * Allocate and initialize domain structure encapsulating
> + * QEMU Process state and monitor connection to QEMU
> + * for completing QMP Queries.
> + */

Oh, so you're creating a fake domain object to be able to start QEMU
process for probing? That's both confusing and fragile. Please, don't do
that. You can just rename, move, and extend the existing
virQEMUCapsInitQMPCommand.

If you need to keep some data across several functions, you should
create a new structure for that.

Jirka