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 :|
