[libvirt] [PATCH v5 08/36] qemu_process: All ProcessQMP errors are fatal

[Jiri Denemark]

On Sun, Dec 02, 2018 at 23:10:02 -0600, Chris Venteicher wrote:
> In the past capabilities could be determined in other ways if QMP
> messaging didn't succeed so a non-fatal error case was included in the
> capabilities and QMP Process code.
> 
> For a while now, QMP capabilities failure has been a fatal case.
> 
> This patch makes QMP process failures return as a fatal error in all
> cases consistent with 1) all failures actually being fatal in
> QMP capabilities code and 2) the QMP process code being made generic.
> 
> The process changes impact the capabilities code because non-fatal
> return codes are no longer returned.
> 
> The rest of the QMP associated capabilities code is updated to make all
> errors fatal for consistency.
> 
> The process changes also force a change in QMP process stderr reporting
> because the previous triggers for stderr reporting are no longer passed
> through the function interfaces.
> 
> As best I can tell the only case where QMP process stderr should be
> explicitly reported is when the QMP process exits with a non-zero
> status.
> 
> Error reporting is moved to virQEMUCapsInitQMP and the QMP process
> stderr is reported only when the QMP process status code is non-zero
> (and is only reported for the primary QMP query not the secondary TCG
>  specific QMP query.)

Looks like the patch is unnecessarily doing more things at once...

> 
> Signed-off-by: Chris Venteicher <cventeic at redhat.com>
> ---
>  src/qemu/qemu_capabilities.c | 83 ++++++++++++++++--------------------
>  src/qemu/qemu_process.c      | 24 ++++-------
>  src/qemu/qemu_process.h      |  1 +
>  3 files changed, 45 insertions(+), 63 deletions(-)
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index bed9ca26a1..1efec85b57 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
...
> @@ -4234,25 +4231,47 @@ virQEMUCapsInitQMPMonitorTCG(virQEMUCapsPtr qemuCaps ATTRIBUTE_UNUSED,
>  }
>  
>  
> +#define MESSAGE_ID_CAPS_PROBE_FAILURE "8ae2f3fb-2dbe-498e-8fbd-012d40afa361"
> +
> +static void
> +virQEMUCapsLogProbeFailure(const char *binary)
> +{
> +    virLogMetadata meta[] = {
> +        { .key = "MESSAGE_ID", .s = MESSAGE_ID_CAPS_PROBE_FAILURE, .iv = 0 },
> +        { .key = "LIBVIRT_QEMU_BINARY", .s = binary, .iv = 0 },
> +        { .key = NULL },
> +    };
> +
> +    virLogMessage(&virLogSelf,
> +                  VIR_LOG_WARN,
> +                  __FILE__, __LINE__, __func__,
> +                  meta,
> +                  _("Failed to probe capabilities for %s: %s"),
> +                  binary, virGetLastErrorMessage());
> +}
> +
> +

I agree calling virQEMUCapsLogProbeFailure from inside
virQEMUCapsInitQMP makes a bit more sense then doing it in the caller,
but it should be done separately. The change has nothing to do with
the purpose of this patch.

>  static int
>  virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps,
>                     const char *libDir,
>                     uid_t runUid,
> -                   gid_t runGid,
> -                   char **qmperr)
> +                   gid_t runGid)

This could be moved to the "qemu_process: Persist stderr in
qemuProcessQmp struct" patch. There should be no reason for splitting
a single logical change in two patches.

>  {
>      qemuProcessQmpPtr proc = NULL;
>      qemuProcessQmpPtr procTCG = NULL;
> +    char *qmperr = NULL;
>      int ret = -1;
> -    int rc;
>  
>      if (!(proc = qemuProcessQmpNew(qemuCaps->binary, libDir,
> -                                   runUid, runGid, qmperr, false)))
> +                                   runUid, runGid, &qmperr, false)))
>          goto cleanup;
>  
> -    if ((rc = qemuProcessQmpRun(proc)) != 0) {
> -        if (rc == 1)
> -            ret = 0;
> +    if (qemuProcessQmpRun(proc) < 0) {
> +        if (proc->status != 0)
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("Failed to probe QEMU binary with QMP: %s"),
> +                           qmperr ? qmperr : _("uknown error"));

[*]

> +
>          goto cleanup;
>      }
>  
> @@ -4270,11 +4289,8 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps,
>          procTCG = qemuProcessQmpNew(qemuCaps->binary, libDir,
>                                      runUid, runGid, NULL, true);
>  
> -        if ((rc = qemuProcessQmpRun(procTCG)) != 0) {
> -            if (rc == 1)
> -                ret = 0;
> +        if (qemuProcessQmpRun(procTCG) < 0)
>              goto cleanup;

This would return -1 without setting any error. The virReportError [*]
from the previous hunk would need to be repeated here. And doing so
would indicate this is a wrong place for reporting the error. The
qemuProcessQmpRun function returned -1 and thus it should have called
virReportError itself.

And as I suggested in my review for the previous version, there's no
need to have two almost identical copies of the same code in
virQEMUCapsInitQMP. Just separate the code into a new function which
will then be called twice in a new patch put before this one. Then you
don't have to change two places and even moving the
virQEMUCapsLogProbeFailure call into virQEMUCapsInitQMP will be simpler.

> -        }
>  
>          if (virQEMUCapsInitQMPMonitorTCG(qemuCaps, procTCG->mon) < 0)
>              goto cleanup;
...
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 324151a7c3..2ec0d5340d 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
...
> @@ -8220,19 +8215,20 @@ qemuProcessQmpRun(qemuProcessQmpPtr proc)
>  
>      virCommandSetErrorBuffer(proc->cmd, proc->qmperr);
>  
> -    /* Log, but otherwise ignore, non-zero status.  */
> -    if (virCommandRun(proc->cmd, &status) < 0)
> +    proc->status = 0;
> +
> +    if (virCommandRun(proc->cmd, &proc->status) < 0)
>          goto cleanup;
>  
> -    if (status != 0) {
> +    if (proc->status != 0) {
>          VIR_DEBUG("QEMU %s exited with status %d: %s",
> -                  proc->binary, status, *proc->qmperr);
> -        goto ignore;
> +                  proc->binary, proc->status, *proc->qmperr);
> +        goto cleanup;

Just report the error [*] here instead of logging a debug message and
there will be no reason to propagate the status up in the call stack or
put it into qemuProcessQmpPtr.

Reporting the error here would also avoid the issue in one of the
following patches: qemuConnectBaselineHypervisorCPU may return -1
without reporting any error if QEMU failed to start.

And obviously the error message will need to be a bit generalized
(later once the function starts to be usable for more then probing
capabilities).

> 
>      if (virPidFileReadPath(proc->pidfile, &proc->pid) < 0) {
>          VIR_DEBUG("Failed to read pidfile %s", proc->pidfile);
> -        goto ignore;
> +        goto cleanup;
>      }

This would return -1 without reporting any error because
virPidFileReadPath returns -errno and does not set any error. Thus
VIR_DEBUG needs to be replaced with

    virReportSystemError(-rc, _("Failed to read pidfile %s"),
                         proc->pidfile);

where rc is the return value of virPidFileReadPath;

...

Jirka