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.
That said, I agree with you that in general we don't want our Free()
function to do anything more than release the allocated memory. To
get there, though, it looks like some reworking is needed... I'll
try to make it work.
--
Andrea Bolognani / Red Hat / Virtualization
