[libvirt] [PATCH RFC 07/22] qemu_process: Use qemuProcess struct for a single process

[Jiri Denemark]

On Sun, Nov 11, 2018 at 13:59:15 -0600, Chris Venteicher wrote:
> In new process code, move from model where qemuProcess struct can be
> used to activate a series of Qemu processes to model where one
> qemuProcess struct is used for one and only one Qemu process.
> 
> The forceTCG parameter (use / don't use KVM) will be passed when the
> qemuProcess struct is initialized since the qemuProcess struct won't be
> reused.
> 
> Signed-off-by: Chris Venteicher <cventeic at redhat.com>
> ---
>  src/qemu/qemu_capabilities.c | 16 ++++++++++++----
>  src/qemu/qemu_process.c      | 11 +++++++----
>  src/qemu/qemu_process.h      |  6 ++++--
>  3 files changed, 23 insertions(+), 10 deletions(-)
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 082874082b..a957c3bdbd 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -4220,14 +4220,16 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps,
>                     char **qmperr)
>  {
>      qemuProcessPtr proc = NULL;
> +    qemuProcessPtr proc_kvm = NULL;

s/proc_kvm/procTCG/

The second QEMU process probes for TCG capabilities in case the first
reported KVM as enabled (otherwise the first one already reported TCG
capabilities).

>      int ret = -1;
>      int rc;
> +    bool forceTCG = false;

This variable does not make a lot of sense. You can just use false/true
directly when calling qemuProcessNew.

>  
>      if (!(proc = qemuProcessNew(qemuCaps->binary, libDir,
> -                                runUid, runGid, qmperr)))
> +                                runUid, runGid, qmperr, forceTCG)))
>          goto cleanup;
>  
> -    if ((rc = qemuProcessRun(proc, false)) != 0) {
> +    if ((rc = qemuProcessRun(proc)) != 0) {
>          if (rc == 1)
>              ret = 0;
>          goto cleanup;
> @@ -4238,13 +4240,17 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps,
>  
>      if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) {
>          qemuProcessStopQmp(proc);
> -        if ((rc = qemuProcessRun(proc, true)) != 0) {
> +
> +        forceTCG = true;
> +        proc_kvm = qemuProcessNew(qemuCaps->binary, libDir, runUid, runGid, NULL, forceTCG);
> +
> +        if ((rc = qemuProcessRun(proc_kvm)) != 0) {
>              if (rc == 1)
>                  ret = 0;
>              goto cleanup;
>          }
>  
> -        if (virQEMUCapsInitQMPMonitorTCG(qemuCaps, proc->mon) < 0)
> +        if (virQEMUCapsInitQMPMonitorTCG(qemuCaps, proc_kvm->mon) < 0)
>              goto cleanup;
>      }
>  
> @@ -4252,7 +4258,9 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps,
>  
>   cleanup:
>      qemuProcessStopQmp(proc);
> +    qemuProcessStopQmp(proc_kvm);
>      qemuProcessFree(proc);
> +    qemuProcessFree(proc_kvm);
>      return ret;
>  }

After this patch virQEMUCapsInitQMP contains two almost identical parts.
It should be further refactored (in a follow up patch) to something like
(I was too lazy to come up with a good name for the function)

    virQEMUCapsInitQMP()
    {
        if (virQEMUCapsInitQMP...(..., false, err) < 0)
            return -1;

        if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM) &&
            virQEMUCapsInitQMP...(..., true, NULL) < 0)
            return -1;

        return 0;
    }

>  
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 2571024e8e..dda74d5b7a 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -8100,7 +8100,8 @@ qemuProcessNew(const char *binary,
>                 const char *libDir,
>                 uid_t runUid,
>                 gid_t runGid,
> -               char **qmperr)
> +               char **qmperr,
> +               bool forceTCG)
>  {
>      qemuProcessPtr proc = NULL;
>  
> @@ -8110,10 +8111,13 @@ qemuProcessNew(const char *binary,
>      if (VIR_STRDUP(proc->binary, binary) < 0)
>          goto error;
>  
> +    proc->forceTCG = forceTCG;

I'd put this just after the "proc->qmperr = qmperr;" line with no empty
line separator to keep the order consistent with the order of function
parameters. But it's not a big deal.

> +
>      proc->runUid = runUid;
>      proc->runGid = runGid;
>      proc->qmperr = qmperr;
>  
> +

Please, delete the extra empty line.

>      /* the ".sock" sufix is important to avoid a possible clash with a qemu
>       * domain called "capabilities"
>       */

Jirka