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

[Daniel P. Berrangé]

On Wed, Feb 13, 2019 at 01:04:07PM +0100, Andrea Bolognani wrote:
> Logging the error is fine and all, but getting the information
> to the user directly is even better.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1578741
> Signed-off-by: Andrea Bolognani <abologna at redhat.com>
> ---
>  src/util/virfile.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/src/util/virfile.c b/src/util/virfile.c
> index 271bf5e796..30cad87df9 100644
> --- a/src/util/virfile.c
> +++ b/src/util/virfile.c
> @@ -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 ?

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