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

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

On 13/05/13 11:45, Ian Campbell wrote:
> On Mon, 2013-05-13 at 10:35 +0100, Roger Pau Monne 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
> 
> xl seems pretty inconsistent in its use of libxl error codes, sometimes
> it uses +ERROR_FOO, other times -ERROR_FOO and other times it uses its
> own open coded numbers.
> 
> So I wouldn't say this is wrong as such but so long as the final xl exit
> code is 0 on success and non-zero otherwise I think it is up to George.

Yes I agree, xl is full of return 1, -1 and things like that, but anyway
I think it was worth mentioning.

>>
>>>      }
>>>  
>>>      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,
> 
> I prefer to have xl clear up properly whenever possible. Not because I
> care particularly about xl (which isn't generally long lived) but
> because it makes it possible to use valgrind+xl to check for leaks in
> libxl, and I care about that because other toolstacks which use libxl
> can be long lived and therefore care about leaks. Having xl not contain
> false positive leaks helps here.
> 
> Ian.
>