Re: [Xen-devel] [PATCH v2 10/15] libxl: add option to choose who executes hotplug scripts

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Roger Pau Monne writes ("[PATCH v2 10/15] libxl: add option to choose who 
executes hotplug scripts"):
> Add and option to xl.conf file to decide if hotplug scripts are
> executed from the toolstack (xl) or from udev as it used to be in the
> past.
>
> +    int hotplug_setting = libxl__hotplug_settings(gc, t);

This function sometimes returns a non-negative boolean, and sometimes
a (negative) libxl error code.  That doesn't seem to be handled
correctly here at the call site.  (And it should be mentioned in a doc
comment.)

> diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
> index b89aef7..bdf14d5 100644
> --- a/tools/libxl/libxl_internal.c
> +++ b/tools/libxl/libxl_internal.c
> @@ -346,6 +346,25 @@ libxl_device_model_version 
> libxl__device_model_version_running(libxl__gc *gc,
>      return value;
>  }
>  
> +int libxl__hotplug_settings(libxl__gc *gc, xs_transaction_t t)
> +{
> +    int rc = 0;
> +    char *val;
> +
> +    val = libxl__xs_read(gc, t, DISABLE_UDEV_PATH);
> +    if (!val && errno != ENOENT) {
> +        LOGE(ERROR, "cannot read %s from xenstore", DISABLE_UDEV_PATH);
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +    if (!val) val = "0";
> +
> +    rc = atoi(val);

This is a bit hazardous.  What if the value in xenstore is "42" or
"-3" ?  At the very least I think you should write:

       rc = !!atoi(val);

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel