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