Re: [libvirt] [PATCH v3 4/4] util: Report error in virFileWrapperFdClose()
On Wed, Feb 20, 2019 at 01:34:15PM +0100, Andrea Bolognani wrote:
> On Tue, 2019-02-19 at 16:58 +, Daniel P. Berrangé wrote:
> > On Tue, Feb 19, 2019 at 05:52:31PM +0100, Andrea Bolognani wrote:
> [...]
> > > @@ -337,8 +337,12 @@ virFileWrapperFdClose(virFileWrapperFdPtr wfd)
> > >
> > > ret = virCommandWait(wfd->cmd, NULL);
> > >
> > > +/* 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);
> >
> > ret needs to be set to -1 in this case
>
> I expect that a command who produced output on stderr also exited
> with a non-zero return code, but explicitly erroring out if the
> former condition is detected can't possibly hurt I guess.
The alternative is to change the conditional to
if (ret != 0 && wfd->err_msg && *wfd->err_msg)
...
If we think there's a risk of the command printing stuff to stderr in
a non-error scenario.
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 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 4/4] util: Report error in virFileWrapperFdClose()
On Tue, 2019-02-19 at 16:58 +, Daniel P. Berrangé wrote:
> On Tue, Feb 19, 2019 at 05:52:31PM +0100, Andrea Bolognani wrote:
[...]
> > @@ -337,8 +337,12 @@ virFileWrapperFdClose(virFileWrapperFdPtr wfd)
> >
> > ret = virCommandWait(wfd->cmd, NULL);
> >
> > +/* 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);
>
> ret needs to be set to -1 in this case
I expect that a command who produced output on stderr also exited
with a non-zero return code, but explicitly erroring out if the
former condition is detected can't possibly hurt I guess.
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 4/4] util: Report error in virFileWrapperFdClose()
On Tue, Feb 19, 2019 at 05:52:31PM +0100, Andrea Bolognani wrote:
> libvirt_iohelper is used internally by the virFileWrapperFd APIs;
> more specifically, in the QEMU driver we have the doCoreDump() and
> qemuDomainSaveMemory() helper functions as users, and those in turn
> end up being called by the implementation of several driver APIs.
>
> By calling virReportError() if libvirt_iohelper has failed, we
> overwrite whatever generic error message QEMU might have raised
> with the more useful one generated by the helper program.
>
> After this commit, the user will be able to see the error directly
> instead of having to dig in the journal or libvirtd log.
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1578741
>
> Signed-off-by: Andrea Bolognani
> ---
> 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 8e045279f0..1b61cbd127 100644
> --- a/src/util/virfile.c
> +++ b/src/util/virfile.c
> @@ -337,8 +337,12 @@ virFileWrapperFdClose(virFileWrapperFdPtr wfd)
>
> ret = virCommandWait(wfd->cmd, NULL);
>
> +/* 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);
ret needs to be set to -1 in this case
>
> wfd->closed = true;
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 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 4/4] util: Report error in virFileWrapperFdClose()
libvirt_iohelper is used internally by the virFileWrapperFd APIs;
more specifically, in the QEMU driver we have the doCoreDump() and
qemuDomainSaveMemory() helper functions as users, and those in turn
end up being called by the implementation of several driver APIs.
By calling virReportError() if libvirt_iohelper has failed, we
overwrite whatever generic error message QEMU might have raised
with the more useful one generated by the helper program.
After this commit, the user will be able to see the error directly
instead of having to dig in the journal or libvirtd log.
https://bugzilla.redhat.com/show_bug.cgi?id=1578741
Signed-off-by: Andrea Bolognani
---
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 8e045279f0..1b61cbd127 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -337,8 +337,12 @@ virFileWrapperFdClose(virFileWrapperFdPtr wfd)
ret = virCommandWait(wfd->cmd, NULL);
+/* 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);
wfd->closed = true;
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
