[libvirt] [PATCH RFC 09/22] qemu_capabilities: Detect caps probe failure by checking monitor ptr

[Michal Privoznik]

On 11/11/2018 08:59 PM, Chris Venteicher wrote:
> Failure to connect to QEMU to probe capabilities is not considered a error case
> and does not result in a negative return value.
> 
> Check for a NULL monitor connection pointer before trying to send capabilities
> commands to QEMU rather than depending on a special positive return value.
> 
> This simplifies logic and is more consistent with the operation of existing
> qemu_process functions.
> 
> A macro is introduced to easily obtain the monitor pointer from the
> qemuProcess structure.
> 
> Signed-off-by: Chris Venteicher <cventeic at redhat.com>
> ---
>  src/qemu/qemu_capabilities.c | 28 ++++++++++++++++++----------
>  src/qemu/qemu_process.c      |  9 +--------
>  src/qemu/qemu_process.h      |  3 +++
>  3 files changed, 22 insertions(+), 18 deletions(-)
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index f5e327097e..fbb4336201 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -4220,43 +4220,51 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps,
>  {
>      qemuProcessPtr proc = NULL;
>      qemuProcessPtr proc_kvm = NULL;
> +    qemuMonitorPtr mon = NULL;
> +    qemuMonitorPtr mon_kvm = NULL;
>      int ret = -1;
> -    int rc;
>      bool forceTCG = false;
>  
>      if (!(proc = qemuProcessNew(qemuCaps->binary, libDir,
>                                  runUid, runGid, forceTCG)))
>          goto cleanup;
>  
> -    if ((rc = qemuProcessRun(proc)) != 0) {
> -        if (rc == 1)
> -            ret = 0;
> +
> +    if (qemuProcessRun(proc) < 0)
> +        goto cleanup;
> +
> +    if (!(mon = QEMU_PROCESS_MONITOR(proc))) {
> +        ret = 0; /* Failure probing QEMU not considered error case */
>          goto cleanup;
>      }
>  
> -    if (virQEMUCapsInitQMPMonitor(qemuCaps, proc->mon) < 0)
> +    /* Pull capabilities from QEMU */
> +    if (virQEMUCapsInitQMPMonitor(qemuCaps, mon) < 0)
>          goto cleanup;
>  
> +    /* Pull capabilities again if KVM supported */
>      if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) {
>          qemuProcessStopQmp(proc);
>  
>          forceTCG = true;
>          proc_kvm = qemuProcessNew(qemuCaps->binary, libDir, runUid, runGid, forceTCG);
>  
> -        if ((rc = qemuProcessRun(proc_kvm)) != 0) {
> -            if (rc == 1)
> -                ret = 0;
> +        if (qemuProcessRun(proc_kvm) < 0)
> +            goto cleanup;
> +
> +        if (!(mon_kvm = QEMU_PROCESS_MONITOR(proc_kvm))) {
> +            ret = 0; /* Failure probing QEMU not considered error case */
>              goto cleanup;
>          }
>  
> -        if (virQEMUCapsInitQMPMonitorTCG(qemuCaps, proc_kvm->mon) < 0)
> +        if (virQEMUCapsInitQMPMonitorTCG(qemuCaps, mon_kvm) < 0)
>              goto cleanup;
>      }
>  
>      ret = 0;
>  
>   cleanup:
> -    if (proc && !proc->mon) {
> +    if (!mon) {
>          char *err = QEMU_PROCESS_ERROR(proc) ? QEMU_PROCESS_ERROR(proc) : _("unknown error");
>  
>          virReportError(VIR_ERR_INTERNAL_ERROR,
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index a741d1cf91..2640ec2b32 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -8148,10 +8148,6 @@ qemuProcessNew(const char *binary,
>  }
>  
>  
> -/* Returns -1 on fatal error,
> - *          0 on success,
> - *          1 when probing QEMU failed
> - */
>  int
>  qemuProcessRun(qemuProcessPtr proc){
>      virDomainXMLOptionPtr xmlopt = NULL;
> @@ -8218,6 +8214,7 @@ qemuProcessRun(qemuProcessPtr proc){
>  
>      virObjectLock(proc->mon);
>  
> + ignore:

How about removing this label completely? I mean, s/goto ignore;/ret =
0; goto cleanup;/

>      ret = 0;
>  
>   cleanup:
> @@ -8226,10 +8223,6 @@ qemuProcessRun(qemuProcessPtr proc){
>      virObjectUnref(xmlopt);
>  
>      return ret;
> -
> - ignore:
> -    ret = 1;
> -    goto cleanup;
>  }
>  
>  
> diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h
> index abd80baf5c..37194e2501 100644
> --- a/src/qemu/qemu_process.h
> +++ b/src/qemu/qemu_process.h
> @@ -235,6 +235,9 @@ struct _qemuProcess {
>  #define QEMU_PROCESS_ERROR(proc) \
>     ((char *) (proc ? proc->qmperr: NULL))
>  
> +#define QEMU_PROCESS_MONITOR(proc) \
> +   ((qemuMonitorPtr) (proc ? proc->mon : NULL))

This looks like an overkill to me. At the only two points where this is
used @proc/@proc_kvm can't be NULL so proc->mon/proc_kvm->mon can be
accessed directly.

> +
>  qemuProcessPtr qemuProcessNew(const char *binary,
>                                const char *libDir,
>                                uid_t runUid,
> 

Michal