[libvirt] [PATCH v2 2/3] virfile: Report error in virFileWrapperFdFree()

[Daniel P. Berrangé]

On Tue, Feb 19, 2019 at 04:42:51PM +0100, Andrea Bolognani wrote:
> On Tue, 2019-02-19 at 13:58 +0000, Daniel P. Berrangé wrote:
> > On Wed, Feb 13, 2019 at 01:04:07PM +0100, Andrea Bolognani wrote:
> [...]
> > > @@ -351,8 +351,12 @@ virFileWrapperFdFree(virFileWrapperFdPtr wfd)
> > >      if (!wfd)
> > >          return;
> > >  
> > > +    /* If the command used to process IO has produced errors, it's fair
> > > +     * to assume those will be more relevant to the user than whatever
> > > +     * eg. QEMU can figure out on its own, so it's okay if we end up
> > > +     * discarding an existing error */
> > >      if (wfd->err_msg && *wfd->err_msg)
> > > -        VIR_WARN("iohelper reports: %s", wfd->err_msg);
> > > +        virReportError(VIR_ERR_OPERATION_FAILED, "%s", wfd->err_msg);
> > 
> > I don't think this is a good place to report errors. We normally
> > write xxxxFree() functions such that they always succeed & don't
> > report errors. Essentially they should just be cleaning up memory
> > and other allocated resources. This is especially true now that
> > we rely on GCC cleanup functions which automagically call
> > virFileWrapperFdFree when the variable goes out of scope.
> > 
> > IOW, if we need to report an error from the io helper, then it
> > needs to be done earlier, pehrpas in virFileWrapperFdClose ?
> 
> As John noted, that's what the original implementation looked like
> but commit b0c3e931 moved the VIR_WARN() call from Close() to Free()
> to avoid the situation where jumping out from a function early
> results in the former not being called.

I think we should really just revert that commit and make sure
the callers will invoke Close() in all paths & then make Close
use virReportError instead of WARN.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|