[libvirt] [PATCH RFC 08/22] qemu_process: Persist stderr in qemuProcess struct

[Jiri Denemark]

On Mon, Nov 12, 2018 at 16:53:34 +0100, Michal Privoznik wrote:
> On 11/11/2018 08:59 PM, Chris Venteicher wrote:
> > A qemuProcess struct tracks the entire lifespan of a single QEMU Process
> > including storing error output when the process terminates or activation
> > fails.
> > 
> > Error output remains available until qemuProcessFree is called.
> > 
> > The qmperr buffer no longer needs to be maintained outside of the
> > qemuProcess structure because the structure is used for a single QEMU
> > process and the structures can be maintained as long as required
> > to retrieve the process error info.
> > 
> > Capabilities init code is refactored but continues to report QEMU
> > process error data only when the initial (non KVM) QEMU process activation
> > fails to result in a usable monitor connection for retrieving
> > capabilities.
> > 
> > The VIR_ERR_INTERNAL_ERROR "Failed to probe QEMU binary" reporting is
> > moved into virQEMUCapsInitQMP function and the stderr string is
> > extracted from the qemuProcess struct using a macro to retrieve the
> > string.
> > 
> > The same error and log message should be generated, in the same
> > conditions, after this patch as before.
> > 
> > Signed-off-by: Chris Venteicher <cventeic at redhat.com>
> > ---
> >  src/qemu/qemu_capabilities.c | 27 ++++++++++++---------------
> >  src/qemu/qemu_process.c      | 12 ++++--------
> >  src/qemu/qemu_process.h      |  6 ++++--
> >  3 files changed, 20 insertions(+), 25 deletions(-)
> > 
> > diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> > index a957c3bdbd..f5e327097e 100644
> > --- a/src/qemu/qemu_capabilities.c
> > +++ b/src/qemu/qemu_capabilities.c
...
> > @@ -4323,15 +4328,8 @@ virQEMUCapsNewForBinaryInternal(virArch hostArch,
> >          goto error;
> >      }
> >  
> > -    if (virQEMUCapsInitQMP(qemuCaps, libDir, runUid, runGid, &qmperr) < 0) {
> > -        virQEMUCapsLogProbeFailure(binary);
> > -        goto error;
> > -    }
> > -
> > -    if (!qemuCaps->usedQMP) {
> > -        virReportError(VIR_ERR_INTERNAL_ERROR,
> > -                       _("Failed to probe QEMU binary with QMP: %s"),
> > -                       qmperr ? qmperr : _("unknown error"));
> > +    if (virQEMUCapsInitQMP(qemuCaps, libDir, runUid, runGid) < 0 ||
> > +        !qemuCaps->usedQMP) {
> >          virQEMUCapsLogProbeFailure(binary);
> 
> Oh, this won't fly. So virReportError() sets the error object and
> virQEMUCapsLogProbeFailure() uses it (by calling
> virGetLastErrorMessage()). But since you're removing the
> virReportError() call then there's no error object to get the error
> message from. IOW this will probably log: "Failed to probe capabilities
> for /usr/bin/qemu: no error" even though later the actual qemu error
> message is logged.

Not really. The virReportError is still there, just moved inside
virQEMUCapsInitQMP. No issue to see here I believe.

Jirka