[libvirt] [PATCH v3 3/4] util: Move error reporting back to virFileWrapperFdClose()

[Andrea Bolognani]

On Tue, 2019-02-19 at 16:59 +0000, Daniel P. Berrangé wrote:
> On Tue, Feb 19, 2019 at 05:52:30PM +0100, Andrea Bolognani wrote:
> > virFileWrapperFdFree(), like all free functions, is supposed
> > to only release allocated resources, so error reporting is
> > better suited for virFileWrapperFdClose().
> > 
> > This reverts most of commit b0c3e931804a.
> > 
> > Signed-off-by: Andrea Bolognani <abologna at redhat.com>
> > ---
> >  src/util/virfile.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/util/virfile.c b/src/util/virfile.c
> > index 42add5a2cd..8e045279f0 100644
> > --- a/src/util/virfile.c
> > +++ b/src/util/virfile.c
> > @@ -337,6 +337,9 @@ virFileWrapperFdClose(virFileWrapperFdPtr wfd)
> >  
> >      ret = virCommandWait(wfd->cmd, NULL);
> >  
> > +    if (wfd->err_msg && *wfd->err_msg)
> > +        VIR_WARN("iohelper reports: %s", wfd->err_msg);
> > +
> >      wfd->closed = true;
> >  
> >      return ret;
> > @@ -357,9 +360,6 @@ virFileWrapperFdFree(virFileWrapperFdPtr wfd)
> >      if (!wfd)
> >          return;
> >  
> > -    if (wfd->err_msg && *wfd->err_msg)
> > -        VIR_WARN("iohelper reports: %s", wfd->err_msg);
> > -
> >      virCommandAbort(wfd->cmd);
> 
> It feels like this could be reverted too. We already call virCommandFree
> which I think should call Abort for us - though not 100% confident since
> the scenario in which "reap = true" is set is quite complex.

According to virCommandRunAsync()'s documentation

  There are two approaches to child process cleanup.
  1. Use auto-cleanup, by passing NULL for pid.  The child will be
  auto-reaped by virCommandFree, unless you reap it earlier via
  virCommandWait or virCommandAbort.

We call virCommandRunAsync() with pid=NULL in virFileWrapperFdNew(),
and then both call virCommandWait() in virFileWrapperFdClose() *and*
virCommandFree() in virFileWrapperFdFree(), so I think you're right
that we don't need to call virCommandAbort() explicitly and we can
simply revert all of b0c3e931804a.

-- 
Andrea Bolognani / Red Hat / Virtualization