On Tue, Feb 19, 2013 at 18:12:13 +0100, Michal Privoznik wrote:
> Currently, if lzop decompression binary produces a warning, it
> doesn't exit with zero status but 2 instead. Terrifying, but
> true. However, warnings may be ignored using '--ignore-warn'
> command line argument. Moreover, in which case, the exit status
> will be zero.
> ---
> src/qemu/qemu_driver.c | 68 ++++++++++++++++++++++++++++++++++----------------
> 1 file changed, 46 insertions(+), 22 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index dc35b91..41c6168 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -2507,6 +2507,36 @@ qemuCompressProgramName(int compress)
> qemuSaveCompressionTypeToString(compress));
> }
>
> +static virCommandPtr
> +qemuCompressGetCommand(virQEMUSaveFormat compression)
> +{
> + virCommandPtr ret = NULL;
> + const char *prog = qemuSaveCompressionTypeToString(compression);
> +
> + if (!prog) {
> + virReportError(VIR_ERR_OPERATION_FAILED,
> + _("Invalid compressed save format %d"),
> + compression);
> + return NULL;
> + }
> +
> + if (!(ret = virCommandNew(prog))) {
> + virReportOOMError();
> + return NULL;
> + }
Wrong, checking the result of virCommandNew is useless. Moreover, you
should move the "-dc" options into this function too, it's quite strange
to see arguments being added in several places. Thus, you can just copy
the original line here:
ret = virCommandNewArgList(prog, "-dc", NULL);
> +
> + switch (compression) {
> + case QEMU_SAVE_FORMAT_LZOP:
> + virCommandAddArg(ret, "--ignore-warn");
> + break;
> + default:
> + /* Don't add nothing for now */
Is this double negative intentional? Should we perhaps rewrite the error
message above as "Ain't no valid compressed save format"? :-)
> + break;
> + }
> +
> + return ret;
> +}
> +
> /* Internal function to properly create or open existing files, with
> * ownership affected by qemu driver setup. */
> static int
> @@ -4775,32 +4805,26 @@ qemuDomainSaveImageStartVM(virConnectPtr conn,
> if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
> goto cleanup;
>
> - if (header->version == 2) {
> - const char *prog = qemuSaveCompressionTypeToString(header->compressed);
> - if (prog == NULL) {
> - virReportError(VIR_ERR_OPERATION_FAILED,
> - _("Invalid compressed save format %d"),
> - header->compressed);
> + if ((header->version == 2) &&
> + (header->compressed != QEMU_SAVE_FORMAT_RAW)) {
> + if (!(cmd = qemuCompressGetCommand(header->compressed)))
> goto cleanup;
> - }
>
> - if (header->compressed != QEMU_SAVE_FORMAT_RAW) {
> - cmd = virCommandNewArgList(prog, "-dc", NULL);
> - intermediatefd = *fd;
> - *fd = -1;
> + intermediatefd = *fd;
> + *fd = -1;
>
> - virCommandSetInputFD(cmd, intermediatefd);
> - virCommandSetOutputFD(cmd, fd);
> - virCommandSetErrorBuffer(cmd, &errbuf);
> - virCommandDoAsyncIO(cmd);
> + virCommandAddArg(cmd, "-dc");
The line above should be moved to qemuCompressGetCommand().
> + virCommandSetInputFD(cmd, intermediatefd);
> + virCommandSetOutputFD(cmd, fd);
> + virCommandSetErrorBuffer(cmd, &errbuf);
> + virCommandDoAsyncIO(cmd);
>
> - if (virCommandRunAsync(cmd, NULL) < 0) {
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("Failed to start decompression binary %s"),
> - prog);
> - *fd = intermediatefd;
> - goto cleanup;
> - }
> + if (virCommandRunAsync(cmd, NULL) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Failed to start decompression binary %s"),
> + qemuCompressProgramName(header->compressed));
I know this is not your fault but we should really avoid overwriting
already reported errors.
> + *fd = intermediatefd;
> + goto cleanup;
> }
> }
Jirka
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list