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

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

[George Dunlap]

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;
     }
 
     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);
+        return 1;
+    }
+
+    if (libxl_cdrom_insert(ctx, domid, &disk, NULL))
+        rc=1;
 
     libxl_device_disk_dispose(&disk);
     free(buf);
+
+    return rc;
 }
 
 int main_cd_eject(int argc, char **argv)
@@ -2539,8 +2556,7 @@ int main_cd_eject(int argc, char **argv)
     domid = find_domain(argv[optind]);
     virtdev = argv[optind + 1];
 
-    cd_insert(domid, virtdev, NULL);
-    return 0;
+    return cd_insert(domid, virtdev, NULL);
 }
 
 int main_cd_insert(int argc, char **argv)
-- 
1.7.9.5

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

[George Dunlap]

On Fri, May 10, 2013 at 4:43 PM, George Dunlap
<george.dunlap@eu.citrix.com> wrote:
> Two changes:
> * Stat the file before calling libxl_cdrom_insert()
> * Return an error if anything fails (including libxl_cdrom_insert)

Hmm -- not sure how "empty file" got into that description -- it
should say "non-existent file"...

 -George

>
> 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;
>      }
>
>      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);
> +        return 1;
> +    }
> +
> +    if (libxl_cdrom_insert(ctx, domid, &disk, NULL))
> +        rc=1;
>
>      libxl_device_disk_dispose(&disk);
>      free(buf);
> +
> +    return rc;
>  }
>
>  int main_cd_eject(int argc, char **argv)
> @@ -2539,8 +2556,7 @@ int main_cd_eject(int argc, char **argv)
>      domid = find_domain(argv[optind]);
>      virtdev = argv[optind + 1];
>
> -    cd_insert(domid, virtdev, NULL);
> -    return 0;
> +    return cd_insert(domid, virtdev, NULL);
>  }
>
>  int main_cd_insert(int argc, char **argv)
> --
> 1.7.9.5
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

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

[Roger Pau Monné]

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

>      }
>  
>      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:

out:
    free(buf);
    return rc;


and go to it.

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

>  
>      libxl_device_disk_dispose(&disk);
>      free(buf);
> +
> +    return rc;
>  }
>  
>  int main_cd_eject(int argc, char **argv)
> @@ -2539,8 +2556,7 @@ int main_cd_eject(int argc, char **argv)
>      domid = find_domain(argv[optind]);
>      virtdev = argv[optind + 1];
>  
> -    cd_insert(domid, virtdev, NULL);
> -    return 0;
> +    return cd_insert(domid, virtdev, NULL);
>  }
>  
>  int main_cd_insert(int argc, char **argv)
> 

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

[Ian Campbell]

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

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

[Roger Pau Monné]

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

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

[George Dunlap]

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

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

[George Dunlap]

On Thu, May 30, 2013 at 10:00 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Tue, 2013-05-14 at 11:07 +0100, 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.
>>
>> v2:
>>  - Avoid leaking buf
>>
>> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
>
> Acked + applied, thanks.
>
>> +    /* 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. */
>
> Is this on your 4.4 list?

Yes:

* libxl: Don't use RAW format for "URL"-based qdisks (e.g., rbd:rbd/foo.img)

We talked about this at the hackathon as well.

 -George

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

[Ian Campbell]

On Tue, 2013-05-14 at 11:07 +0100, 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.
> 
> v2:
>  - Avoid leaking buf
> 
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>

Acked + applied, thanks.

> +    /* 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. */

Is this on your 4.4 list?

Ian.

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

[George Dunlap]

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.

v2:
 - Avoid leaking buf

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 |   29 ++++++++++++++++++++++++-----
 1 file changed, 24 insertions(+), 5 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index c1a969b..2ef6875 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -2505,25 +2505,45 @@ 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;
+        rc = 1;
+        goto out;
     }
 
     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);
+        rc = 1;
+        goto out;
+    }
+
+    if (libxl_cdrom_insert(ctx, domid, &disk, NULL))
+        rc=1;
 
+out:
     libxl_device_disk_dispose(&disk);
     free(buf);
+
+    return rc;
 }
 
 int main_cd_eject(int argc, char **argv)
@@ -2539,8 +2559,7 @@ int main_cd_eject(int argc, char **argv)
     domid = find_domain(argv[optind]);
     virtdev = argv[optind + 1];
 
-    cd_insert(domid, virtdev, NULL);
-    return 0;
+    return cd_insert(domid, virtdev, NULL);
 }
 
 int main_cd_insert(int argc, char **argv)
-- 
1.7.9.5