Re: [PATCH v6] libxl: allow 'phy' backend to use empty files

["Roger Pau Monné" <roger.pau@citrix.com>]

El 19/2/16 a les 18:30, Ian Jackson ha escrit:
> Roger Pau Monne writes ("[PATCH v6] libxl: allow 'phy' backend to use empty files"):
>> This was introduced by 97ee1f (~5 years ago), but was probably never
>> surfaced because most people used regular files as CDROM images, so the PHY
>> backend was actually never selected. A year ago this was changed, and now
>> regular RAW files are also handled by the PHY backend, which has made this
>> bug surface.
>>
>> Fix it by allowing empty disks to use the PHY backend, skipping the stat
>> tests.
>>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> Reported-by: Alex Braunegg <alex.braunegg@gmail.com>
> 
> Thanks, and thanks to Alex for the testing, but:
> 
>> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
>> index 8bb5e93..2e08108 100644
>> --- a/tools/libxl/libxl_device.c
>> +++ b/tools/libxl/libxl_device.c
>> @@ -196,6 +196,14 @@ static int disk_try_backend(disk_try_backend_args *a,
>>              goto bad_format;
>>          }
>>  
>> +        if (a->disk->format == LIBXL_DISK_FORMAT_EMPTY) {
>> +            assert(a->disk->pdev_path == NULL ||
>> +                   !strcmp(a->disk->pdev_path, ""));
> 
> I agree that these things ought to be true but I don't see what code
> in libxl ensures that they definitely are.
> 
> I think this check ought to be moved to libxl__device_disk_set_backend,
> or perhaps even earlier, and should generate an ERROR_INVAL rather
> than an assertion failure.

Thanks, libxl can return EINVAL without problems from
libxl__device_disk_set_backend, so I think it's a fine place to put this
check. libxl__device_disk_setdefault should also be a good place, but I
feel _backend is where it makes more sense. v7 is on the way.

Roger.