Re: [PATCH] xl: Return an error if an empty file is passed to cd-insert

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

On 13/05/13 10:35, Roger Pau Monné wrote:
> On 10/05/13 17:43, George Dunlap wrote:
>> Two changes:
>> * Stat the file before calling libxl_cdrom_insert()
>> * Return an error if anything fails (including libxl_cdrom_insert)
>>
>> This is in part to work around the fact that the RAW disk type
>> is used for things that aren't actually files; so we can't call
>> stat in libxl_device.c:libxl__device_disk_set_backend() because
>> it may be going over a remote protocol.
>>
>> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
>> CC: Ian Campbell <ian.campbell@citrix.com>
>> CC: Ian Jackson <ian.jackson@citrix.com>
>> ---
>>   tools/libxl/xl_cmdimpl.c |   26 +++++++++++++++++++++-----
>>   1 file changed, 21 insertions(+), 5 deletions(-)
>>
>> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
>> index c1a969b..e8ce35b 100644
>> --- a/tools/libxl/xl_cmdimpl.c
>> +++ b/tools/libxl/xl_cmdimpl.c
>> @@ -2505,25 +2505,42 @@ int main_memset(int argc, char **argv)
>>       return 0;
>>   }
>>   
>> -static void cd_insert(uint32_t domid, const char *virtdev, char *phys)
>> +static int cd_insert(uint32_t domid, const char *virtdev, char *phys)
>>   {
>>       libxl_device_disk disk; /* we don't free disk's contents */
>>       char *buf = NULL;
>>       XLU_Config *config = 0;
>> +    struct stat b;
>> +    int rc = 0;
>>   
>>   
>>       if (asprintf(&buf, "vdev=%s,access=r,devtype=cdrom,target=%s",
>>                    virtdev, phys ? phys : "") < 0) {
>>           fprintf(stderr, "out of memory\n");
>> -        return;
>> +        return 1;
> ERROR_NOMEM

If this were a library function I would definitely return -ERROR_NOMEM; 
but since this is just a helper function whose value is going to be 
returned to the command-line, I think just returning 1 is the best thing 
to do.

>
>>       }
>>   
>>       parse_disk_config(&config, buf, &disk);
>>   
>> -    libxl_cdrom_insert(ctx, domid, &disk, NULL);
>> +    /* ATM the existence of the backing file is not checked for qdisk
>> +     * in libxl_cdrom_insert() because RAW is used for remote
>> +     * protocols as well as plain files.  This will ideally be changed
>> +     * for 4.4, but this work-around fixes the problem of "cd-insert"
>> +     * returning success for non-existent files. */
>> +    if (disk.format != LIBXL_DISK_FORMAT_EMPTY
>> +        && stat(disk.pdev_path, &b)) {
>> +        fprintf(stderr, "Cannot stat file: %s\n",
>> +                disk.pdev_path);
> You are leaking buf here, I know xl is just about to exit, but it might
> be best to to define a label below that contains:

Oops!  Sorry about that...

>> +        return 1;
> ERROR_INVAL
>
>> +    }
>> +
>> +    if (libxl_cdrom_insert(ctx, domid, &disk, NULL))
>> +        rc=1;
> You could use the return value of libxl_cdrom_insert instead of 1.

I sort of assumed that libxl_cdrom_insert() was a negative value, and 
that I wanted xl to return 1.

I'll send a respin w/ the leak fixed.

  -George