[libvirt] [PATCH 0/3] qemu: Report better error on dump/migrate failure
A detailed explanation of why this is needed can be found at https://bugzilla.redhat.com/show_bug.cgi?id=1578741#c8 Andrea Bolognani (3): vircommand: Ensure buffers are NULL-terminated virfile: Report error in virFileWrapperFdFree() iohelper: Don't include newlines in error messages src/util/iohelper.c | 2 +- src/util/vircommand.c | 2 ++ src/util/virfile.c | 6 +++++- 3 files changed, 8 insertions(+), 2 deletions(-) -- 2.20.1
The memory allocated by VIR_REALLOC_N() is uninitialized, which means it's not possible to figure out whether any output was produced at all after the fact. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/util/vircommand.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/util/vircommand.c b/src/util/vircommand.c index d965068369..6e9e56d0c0 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -2057,11 +2057,13 @@ virCommandProcessIO(virCommandPtr cmd) outfd = cmd->outfd; if (VIR_REALLOC_N(*cmd->outbuf, 1) < 0) ret = -1; + *cmd->outbuf[0] = '\0'; } if (cmd->errbuf) { errfd = cmd->errfd; if (VIR_REALLOC_N(*cmd->errbuf, 1) < 0) ret = -1; + *cmd->errbuf[0] = '\0'; } if (ret == -1) goto cleanup; -- 2.20.1
On Tue, Feb 05, 2019 at 04:16:21PM +0100, Andrea Bolognani wrote:
The memory allocated by VIR_REALLOC_N() is uninitialized, which means it's not possible to figure out whether any output was produced at all after the fact.
I really wish we had never added the VIR_REALLOC_N function. One of the best things about VIR_ALLOC/VIR_EXPAND/VIR_RESIZE are that they remove all bugs related to use of uninitialized memory. We really ought to try to eliminate use of VIR_REALLOC_N in favour of the other safer functions throughout the code.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/util/vircommand.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/src/util/vircommand.c b/src/util/vircommand.c index d965068369..6e9e56d0c0 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -2057,11 +2057,13 @@ virCommandProcessIO(virCommandPtr cmd) outfd = cmd->outfd; if (VIR_REALLOC_N(*cmd->outbuf, 1) < 0) ret = -1; + *cmd->outbuf[0] = '\0'; } if (cmd->errbuf) { errfd = cmd->errfd; if (VIR_REALLOC_N(*cmd->errbuf, 1) < 0) ret = -1; + *cmd->errbuf[0] = '\0';
Here we don't really care about the original contents on outbuf/errbuf. I'd probably go for making that explicit by replacing VIR_REALLOC_N with VIR_FREE + VIR_ALLOC_N 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 :|
On Tue, 2019-02-05 at 15:23 +0000, Daniel P. Berrangé wrote:
On Tue, Feb 05, 2019 at 04:16:21PM +0100, Andrea Bolognani wrote:
The memory allocated by VIR_REALLOC_N() is uninitialized, which means it's not possible to figure out whether any output was produced at all after the fact.
I really wish we had never added the VIR_REALLOC_N function. One of the best things about VIR_ALLOC/VIR_EXPAND/VIR_RESIZE are that they remove all bugs related to use of uninitialized memory. We really ought to try to eliminate use of VIR_REALLOC_N in favour of the other safer functions throughout the code.
That should be reasonably simple to do, since the functions we'd be moving to offer more guarantees than the ones we'd be moving away from. There are quite a few call sites, though :) [...]
@@ -2057,11 +2057,13 @@ virCommandProcessIO(virCommandPtr cmd) outfd = cmd->outfd; if (VIR_REALLOC_N(*cmd->outbuf, 1) < 0) ret = -1; + *cmd->outbuf[0] = '\0'; } if (cmd->errbuf) { errfd = cmd->errfd; if (VIR_REALLOC_N(*cmd->errbuf, 1) < 0) ret = -1; + *cmd->errbuf[0] = '\0';
Here we don't really care about the original contents on outbuf/errbuf. I'd probably go for making that explicit by replacing VIR_REALLOC_N with VIR_FREE + VIR_ALLOC_N
Sure, can do. -- Andrea Bolognani / Red Hat / Virtualization
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@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); virCommandAbort(wfd->cmd); -- 2.20.1
The newline was pretty arbitrary, and we're better off without it. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/util/iohelper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/iohelper.c b/src/util/iohelper.c index 1ff4a7b314..aed7ef3184 100644 --- a/src/util/iohelper.c +++ b/src/util/iohelper.c @@ -236,7 +236,7 @@ main(int argc, char **argv) return 0; error: - fprintf(stderr, _("%s: failure with %s\n: %s"), + fprintf(stderr, _("%s: failure with %s: %s"), program_name, path, virGetLastErrorMessage()); exit(EXIT_FAILURE); } -- 2.20.1
participants (2)
-
Andrea Bolognani -
Daniel P. Berrangé