Re: [PATCH v2 3/5] qemu_passt: Make passt report errors to stderr whenever possible

[Stefano Brivio <sbrivio@redhat.com>]

On Thu, 16 Feb 2023 17:27:11 +0100
Michal Prívozník <mprivozn@redhat.com> wrote:

> On 2/16/23 17:07, Stefano Brivio wrote:
> > On Thu, 16 Feb 2023 14:32:50 +0100
> > Michal Privoznik <mprivozn@redhat.com> wrote:
> >   
> >> Passt has '--stderr' argument which makes it report error onto
> >> stderr rather to system log. Unfortunately, it's currently
> >> impossible to use both '--log-file' and '--stderr', so pass the
> >> latter only if the former isn't passed. Then, use the stderr to
> >> produce more user friendly error message on failed start.
> >>
> >> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> >> ---
> >>  src/qemu/qemu_passt.c | 22 +++++++++++++++++++---
> >>  1 file changed, 19 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/src/qemu/qemu_passt.c b/src/qemu/qemu_passt.c
> >> index c082c149cd..881205449b 100644
> >> --- a/src/qemu/qemu_passt.c
> >> +++ b/src/qemu/qemu_passt.c
> >> @@ -171,8 +171,13 @@ qemuPasstStart(virDomainObj *vm,
> >>      if (net->sourceDev)
> >>          virCommandAddArgList(cmd, "--interface", net->sourceDev, NULL);
> >>  
> >> -    if (net->backend.logFile)
> >> +    if (net->backend.logFile) {
> >>          virCommandAddArgList(cmd, "--log-file", net->backend.logFile, NULL);
> >> +    } else {
> >> +        /* By default, passt logs into system logger. But we are interested
> >> +         * into errors too. Make it print errors onto stderr. */
> >> +        virCommandAddArg(cmd, "--stderr");
> >> +    }  
> > 
> > There's no need for this, see my previous email, archived at:
> >   https://listman.redhat.com/archives/libvir-list/2023-February/237880.html
> >   
> >>  
> >>      /* Add IP address info */
> >>      for (i = 0; i < net->guestIP.nips; i++) {
> >> @@ -265,8 +270,19 @@ qemuPasstStart(virDomainObj *vm,
> >>          goto error;
> >>  
> >>      if (cmdret < 0 || exitstatus != 0) {
> >> -        virReportError(VIR_ERR_INTERNAL_ERROR,
> >> -                       _("Could not start 'passt': %s"), NULLSTR(errbuf));
> >> +        if (STRNEQ_NULLABLE(errbuf, "")) {
> >> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> >> +                           _("Could not start 'passt': %s"), errbuf);
> >> +        } else if (net->backend.logFile) {
> >> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> >> +                           _("Could not start 'passt': look into %s for error"),
> >> +                           net->backend.logFile);  
> > 
> > ...and this won't be needed either, with Laine's series for passt. It
> > might actually be a bit misleading.
> >   
> >> +        } else {
> >> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> >> +                           _("Could not start 'passt': exit status = '%d'"),
> >> +                           exitstatus);
> >> +        }
> >> +
> >>          goto error;
> >>      }
> >>    
> > 
> > So all in all I think this is unnecessary, but also kind of harmless.
> >   
> 
> Except those patches are not merged yet.

Merged.

> And as we are getting close to
> the release I'd like to make this work with what we have now. We've been
> burned plenty of times with QEMU to learn our lesson. We've merged
> patches that were based not on QEMU's git, but some patches on top
> thinking - yeah, the API won't change. And then it did.
> 
> Now we don't require a release (which would be ideal), but at least for
> patches to be merged. When they get merged then yeah, this can be reworked.

-- 
Stefano