[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] qemu: Run lzop with '--ignore-warn'



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]