[libvirt] [PATCH v2 2/2] confirm compression program availability

[Daniel P. Berrange]

On Mon, Oct 25, 2010 at 09:05:29AM +0900, KAMEZAWA Hiroyuki wrote:
> 
> At compression, external programs are used but it is not checked whether
> the program is available or not.
> Check it at parsing qemu.conf and find problems in early stage.

The problem with doing the error check here is that it will
cause the entire libvirtd to fail to startup, and just syslog
the error. We've found admins often miss these types of problem
in syslog and just file bugs complaining that libvirtd doesn't
start.

I think we should put an explicit check for existance of the
compression program in the API code instead. That way the
error message will get fed to the end user who will easily
notice it.

> 
> ---
>  src/qemu/qemu_conf.c |   46 +++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 39 insertions(+), 7 deletions(-)
> 
> Index: libvirt-0.8.4/src/qemu/qemu_conf.c
> ===================================================================
> --- libvirt-0.8.4.orig/src/qemu/qemu_conf.c
> +++ libvirt-0.8.4/src/qemu/qemu_conf.c
> @@ -316,22 +316,54 @@ int qemudLoadDriverConfig(struct qemud_d
>      p = virConfGetValue (conf, "save_image_format");
>      CHECK_TYPE ("save_image_format", VIR_CONF_STRING);
>      if (p && p->str) {
> -        VIR_FREE(driver->saveImageFormat);
> -        if (!(driver->saveImageFormat = strdup(p->str))) {
> -            virReportOOMError();
> -            virConfFree(conf);
> -            return -1;
> -        }
> +	int find = 1;
> +	if (strcmp(p->str, "raw")) {
> +	    char *c;
> +	    c = virFindFileInPath(p->str);
> +	    if (!c)
> +	        find = 0;
> +	    else
> +	      VIR_FREE(c);
> +	}
> +	VIR_FREE(driver->saveImageFormat);
> +	if (find) {
> +            if (!(driver->saveImageFormat = strdup(p->str))) {
> +	        virReportOOMError();
> +		virConfFree(conf);
> +		return -1;
> +	    }
> +	} else {
> +	    qemuReportError(VIR_ERR_INTERNAL_ERROR,
> +                    "save_image_format cannot find program %s", p->str);
> +	    virConfFree(conf);
> +	    return -1;
> +	}
>      }
>  
>      p = virConfGetValue (conf, "dump_image_format");
>      CHECK_TYPE ("dump_image_format", VIR_CONF_STRING);
>      if (p && p->str) {
> +	int find = 1;
> +	if (strcmp(p->str, "raw")) {
> +            char *c;
> +	    c = virFindFileInPath(p->str);
> +	    if (!c)
> +	        find = 0;
> +	    else
> +	        VIR_FREE(c);
> +	}
>  	VIR_FREE(driver->dumpImageFormat);
> -	if (!(driver->dumpImageFormat = strdup(p->str))) {
> +	if (find) {
> +	    if (!(driver->dumpImageFormat = strdup(p->str))) {
>  		virReportOOMError();
>  		virConfFree(conf);
>  		return -1;
> +	    }
> +	} else {
> +	    qemuReportError(VIR_ERR_INTERNAL_ERROR,
> +                    "dump_image_format cannot find program %s", p->str);
> +	    virConfFree(conf);
> +	    return -1;
>  	}
>      }


Daniel
-- 
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|