Re: [Xen-devel] [PATCH] libxl: Make an internal function explicitly check existence of expected paths

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2012-11-30 at 17:13 +0000, George Dunlap wrote:
> # HG changeset patch
> # User George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> # Date 1354294821 0
> # Node ID bc2a7645dc2be4a01f2b5ee30d7453cc3d7339aa
> # Parent  bd041b7426fe10a730994edd98708ff98ae1cb74
> libxl: Make an internal function explicitly check existence of expected paths
> 
> libxl__device_disk_from_xs_be() was failing without error for some
> missing xenstore nodes in a backend, while assuming (without checking)
> that other nodes were valid, causing a crash when another internal
> error wrote these nodes in the wrong place.
> 
> Make this function consistent by:
> * Checking the existence of all nodes before using
> * Choosing a default only when the node is not written in device_disk_add()
> * Failing with log msg if any node written by device_disk_add() is not present
> * Returning an error on failure
> 
> Also make the callers of the function pay attention to the error and
> behave appropriately.

If libxl__device_disk_from_xs_be returns an error then someone needs to
cleanup the partial allocations in the disk (pdev_path) probably by
calling libxl_device_disk_dispose.

It's probably easiest to do this in libxl__device_disk_from_xs_be on
error rather than modifying all the callers?

Also libxl__append_disk_list_of_type updates *ndisks early, so if you
abort half way through initialising the elements of the disks array
using libxl__device_disk_from_xs_be then the caller will try and free
some stuff which hasn't been initialised. I think the code needs to
remember ndisks-in-array separately from
ndisks-which-I-have-initialised, with the latter becoming the returned
*ndisks.

> v2:
>  * Remove "Internal error", as the failure will most likely look internal
>  * Use LOG(ERROR...) macros for incrased prettiness

More crass?

> @@ -2186,21 +2187,36 @@ static void libxl__device_disk_from_xs_b
>      } else {
>          disk->pdev_path = tmp;
>      }
> -    libxl_string_to_backend(ctx,
> -                        libxl__xs_read(gc, XBT_NULL,
> -                                       libxl__sprintf(gc, "%s/type", 
> be_path)),
> -                        &(disk->backend));
> +
> +    
> +    tmp = libxl__xs_read(gc, XBT_NULL,
> +                         libxl__sprintf(gc, "%s/type", be_path));
> +    if (!tmp) {
> +        LOG(ERROR, "Missing xenstore node %s/type", be_path);
> +        return ERROR_FAIL;
> +    }

I've just remembered about libxl__xs_read_checked which effectively
implements the error reporting for you.

Oh, but it accepts ENOENT, so not quite what you need -- nevermind!

Ian.



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