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

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.