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

[George Dunlap <george.dunlap@eu.citrix.com>]

On 04/12/12 14:35, Ian Campbell wrote:
> On Fri, 2012-11-30 at 17:13 +0000, George Dunlap wrote:
>> # HG changeset patch
>> # User George Dunlap <george.dunlap@eu.citrix.com>
>> # 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?

Well, there are only two callers, only one of which (it looks like) 
needs a clean-up.  It seems like better design to make each caller do 
its own clean-up.  Let me take a look at that.

  -George

>
> 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.
>
>