[PATCH] xl: Perform minimal validation of virtual disk file while parsing config file

[PATCH] xl: Perform minimal validation of virtual disk file while parsing config file

[Kamala Narasimhan]

This patch performs some very basic validation on the virtual disk
file passed through the config file.  This validation ensures that we
don't go too far with the initialization like spawn qemu and more
while there could be some potentially fundamental issues.  Obviously,
there is a lot of room for improvement in the kind of validations we
could do but the below is a minimal first stab at it.  Please consider
this for inclusion or feel free to tweak it as necessary.

Signed-off-by: Kamala Narasimhan <kamala.narasimhan@citrix.com>

Kamala


diff -r ce208811f540 tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c  Thu Jan 13 01:26:44 2011 +0000
+++ b/tools/libxl/xl_cmdimpl.c  Thu Jan 13 10:16:57 2011 -0500
@@ -432,6 +432,35 @@ static int parse_action_on_shutdown(cons
 #define DSTATE_RW        5
 #define DSTATE_TERMINAL  6

+static int validate_virtual_disk(char *file_name, libxl_disk_phystype
disk_type)
+{
+    struct stat stat_buf;
+
+    if ( file_name == NULL )
+    {
+        fprintf(stderr, "Virtual disk file name is NULL!\n");
+        return 0;
+    }
+
+    if ( stat(file_name, &stat_buf) != 0 ) {
+        fprintf(stderr, "Stat on virtual disk %s returned error - \"%s\".\n",
+            file_name, strerror(errno));
+        return 0;
+    }
+    if ( disk_type == PHYSTYPE_PHY ) {
+        if ( !(S_ISBLK(stat_buf.st_mode)) ) {
+            fprintf(stderr, "Virtual disk %s is not a block device!\n",
+                file_name);
+            return 0;
+        }
+    }else if ( stat_buf.st_size == 0 ) {
+        fprintf(stderr, "Virtual disk %s size is 0!\n", file_name);
+        return 0;
+    }
+
+    return 1;
+}
+
 static int parse_disk_config(libxl_device_disk *disk, char *buf2)
 {
     int state = DSTATE_INITIAL;
@@ -485,6 +514,9 @@ static int parse_disk_config(libxl_devic

                 *p = '\0';
                 disk->physpath = (*tok) ? strdup(tok) : NULL;
+                if ( validate_virtual_disk(disk->physpath,
disk->phystype) == 0 )
+                    return 0;
+
                 tok = p + 1;

                 /* hack for ioemu disk spec */

Re: [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file

[Ian Campbell]

On Thu, 2011-01-13 at 15:35 +0000, Kamala Narasimhan wrote:
> This patch performs some very basic validation on the virtual disk
> file passed through the config file.  This validation ensures that we
> don't go too far with the initialization like spawn qemu and more
> while there could be some potentially fundamental issues.  Obviously,
> there is a lot of room for improvement in the kind of validations we
> could do but the below is a minimal first stab at it.  Please consider
> this for inclusion or feel free to tweak it as necessary.
> 
> Signed-off-by: Kamala Narasimhan <kamala.narasimhan@citrix.com>

I wonder if the validation function should be part of libxl?

> Kamala
> 
> 
> diff -r ce208811f540 tools/libxl/xl_cmdimpl.c
> --- a/tools/libxl/xl_cmdimpl.c  Thu Jan 13 01:26:44 2011 +0000
> +++ b/tools/libxl/xl_cmdimpl.c  Thu Jan 13 10:16:57 2011 -0500
> @@ -432,6 +432,35 @@ static int parse_action_on_shutdown(cons
>  #define DSTATE_RW        5
>  #define DSTATE_TERMINAL  6
> 
> +static int validate_virtual_disk(char *file_name, libxl_disk_phystype
> disk_type)
> +{
> +    struct stat stat_buf;
> +
> +    if ( file_name == NULL )
> +    {
> +        fprintf(stderr, "Virtual disk file name is NULL!\n");
> +        return 0;
> +    }
> +
> +    if ( stat(file_name, &stat_buf) != 0 ) {
> +        fprintf(stderr, "Stat on virtual disk %s returned error - \"%s\".\n",
> +            file_name, strerror(errno));
> +        return 0;
> +    }
> +    if ( disk_type == PHYSTYPE_PHY ) {
> +        if ( !(S_ISBLK(stat_buf.st_mode)) ) {
> +            fprintf(stderr, "Virtual disk %s is not a block device!\n",
> +                file_name);
> +            return 0;
> +        }
> +    }else if ( stat_buf.st_size == 0 ) {
> +        fprintf(stderr, "Virtual disk %s size is 0!\n", file_name);
> +        return 0;
> +    }
> +
> +    return 1;
> +}
> +
>  static int parse_disk_config(libxl_device_disk *disk, char *buf2)
>  {
>      int state = DSTATE_INITIAL;
> @@ -485,6 +514,9 @@ static int parse_disk_config(libxl_devic
> 
>                  *p = '\0';
>                  disk->physpath = (*tok) ? strdup(tok) : NULL;
> +                if ( validate_virtual_disk(disk->physpath,
> disk->phystype) == 0 )
> +                    return 0;
> +
>                  tok = p + 1;
> 
>                  /* hack for ioemu disk spec */
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file

[Kamala Narasimhan]

On Fri, Jan 14, 2011 at 4:05 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Thu, 2011-01-13 at 15:35 +0000, Kamala Narasimhan wrote:
>> This patch performs some very basic validation on the virtual disk
>> file passed through the config file.  This validation ensures that we
>> don't go too far with the initialization like spawn qemu and more
>> while there could be some potentially fundamental issues.  Obviously,
>> there is a lot of room for improvement in the kind of validations we
>> could do but the below is a minimal first stab at it.  Please consider
>> this for inclusion or feel free to tweak it as necessary.
>>
>> Signed-off-by: Kamala Narasimhan <kamala.narasimhan@citrix.com>
>
> I wonder if the validation function should be part of libxl?
>

We are better off performing these checks early on as they are very
basic.  Getting far enough to spawn qemu and getting to its block
device initialization code and failing there is a bit of a chase when
it comes to troubleshooting these issues, the cause of which are
rather trivial.  That said, in the long run we might want to move
these validations to upstream qemu as qemu also must perform these
checks especially when run without an accelerator (as there wouldn't
be a toolstack to perform these checks for it in that case).  But,
till that is accomplished these checks need to be somewhere and libxl
seem like a reasonable place in my opinion.

Kamala

Re: [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file

[Gianni Tedesco]

On Fri, 2011-01-14 at 14:55 +0000, Kamala Narasimhan wrote:
> On Fri, Jan 14, 2011 at 4:05 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > On Thu, 2011-01-13 at 15:35 +0000, Kamala Narasimhan wrote:
> >> This patch performs some very basic validation on the virtual disk
> >> file passed through the config file.  This validation ensures that we
> >> don't go too far with the initialization like spawn qemu and more
> >> while there could be some potentially fundamental issues.  Obviously,
> >> there is a lot of room for improvement in the kind of validations we
> >> could do but the below is a minimal first stab at it.  Please consider
> >> this for inclusion or feel free to tweak it as necessary.
> >>
> >> Signed-off-by: Kamala Narasimhan <kamala.narasimhan@citrix.com>
> >
> > I wonder if the validation function should be part of libxl?
> >
> 
> We are better off performing these checks early on as they are very
> basic.  Getting far enough to spawn qemu and getting to its block
> device initialization code and failing there is a bit of a chase when
> it comes to troubleshooting these issues, the cause of which are
> rather trivial.  That said, in the long run we might want to move
> these validations to upstream qemu as qemu also must perform these
> checks especially when run without an accelerator (as there wouldn't
> be a toolstack to perform these checks for it in that case).  But,
> till that is accomplished these checks need to be somewhere and libxl
> seem like a reasonable place in my opinion.

I think Ians point is that your change affects the 'xl' binary and not
the libxl.so library.

Perhaps libxl_device_disk_add() and libxl_cdrom_insert() would be the
reasonable places to add this, replacing fprintf() calls with libxl
logging functions.

Gianni

Re: [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file

[Kamala Narasimhan]

> I think Ians point is that your change affects the 'xl' binary and not
> the libxl.so library.
>
Ah!  I mistook it then.

> Perhaps libxl_device_disk_add() and libxl_cdrom_insert() would be the
> reasonable places to add this, replacing fprintf() calls with libxl
> logging functions.
>
I will make necessary changes and resend the patch.

Kamala

Re: [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file

[Kamala Narasimhan]

> I will make necessary changes and resend the patch.
>

Here is a modified patch for further review and to apply if ok.

Signed-off-by: Kamala Narasimhan <kamala.narasimhan@citrix.com>

diff -r fe8a177ae9cb tools/libxl/libxl.c
--- a/tools/libxl/libxl.c       Wed Jan 19 15:29:04 2011 +0000
+++ b/tools/libxl/libxl.c       Wed Jan 19 12:41:11 2011 -0500
@@ -826,6 +826,41 @@ skip_autopass:

 /******************************************************************************/

+static int validate_virtual_disk(char *file_name, libxl_disk_phystype
disk_type)
+{
+    struct stat stat_buf;
+
+    if ( file_name == NULL ) {
+        fprintf(stderr, "Virtual disk file name is NULL!\n");
+        return 0;
+    }
+
+    /* Return without further validation for empty cdrom drive.
+       Note: Post 4.1 we need to change the interface to handle empty
+       cdrom rather than go with the below assumption.
+     */
+    if ( (strncmp(file_name, "", sizeof("")) == 0) && (disk_type ==
PHYSTYPE_PHY) )
+        return 1;
+
+    if ( stat(file_name, &stat_buf) != 0 ) {
+        fprintf(stderr, "Stat on virtual disk %s returned error - \"%s\".\n",
+            file_name, strerror(errno));
+        return 0;
+    }
+    if ( disk_type == PHYSTYPE_PHY ) {
+        if ( !(S_ISBLK(stat_buf.st_mode)) ) {
+            fprintf(stderr, "Virtual disk %s is not a block device!\n",
+                file_name);
+            return 0;
+        }
+    }else if ( stat_buf.st_size == 0 ) {
+        fprintf(stderr, "Virtual disk %s size is 0!\n", file_name);
+        return 0;
+    }
+
+    return 1;
+}
+
 int libxl_device_disk_add(libxl_ctx *ctx, uint32_t domid,
libxl_device_disk *disk)
 {
     libxl__gc gc = LIBXL_INIT_GC(ctx);
@@ -835,6 +870,9 @@ int libxl_device_disk_add(libxl_ctx *ctx
     int devid;
     libxl__device device;
     int major, minor, rc;
+
+    if ( validate_virtual_disk(disk->physpath, disk->phystype) == 0 )
+        return ERROR_FAIL;

     front = flexarray_make(16, 1);
     if (!front) {

Re: [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file

[Kamala Narasimhan]

Apologies.  I inadvertently neglected Gianni's suggestion to switch to
logging from fprintf.

Signed-off-by: Kamala Narasimhan <kamala.narasimhan@citrix.com>

Kamala

diff -r fe8a177ae9cb tools/libxl/libxl.c
--- a/tools/libxl/libxl.c       Wed Jan 19 15:29:04 2011 +0000
+++ b/tools/libxl/libxl.c       Wed Jan 19 13:23:16 2011 -0500
@@ -826,6 +826,41 @@ skip_autopass:

 /******************************************************************************/

+static int validate_virtual_disk(libxl_ctx *ctx, char *file_name,
libxl_disk_phystype disk_type)
+{
+    struct stat stat_buf;
+
+    if ( file_name == NULL ) {
+        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Virtual disk file name is NULL!\n");
+        return 0;
+    }
+
+    /* Return without further validation for empty cdrom drive.
+       Note: Post 4.1 we need to change the interface to handle empty
+       cdrom rather than go with the below assumption.
+     */
+    if ( (strncmp(file_name, "", sizeof("")) == 0) && (disk_type ==
PHYSTYPE_PHY) )
+        return 1;
+
+    if ( stat(file_name, &stat_buf) != 0 ) {
+        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Stat on virtual disk %s
returned error - \"%s\".\n",
+            file_name, strerror(errno));
+        return 0;
+    }
+    if ( disk_type == PHYSTYPE_PHY ) {
+        if ( !(S_ISBLK(stat_buf.st_mode)) ) {
+            LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Virtual disk %s is not
a block device!\n",
+                file_name);
+            return 0;
+        }
+    } else if ( stat_buf.st_size == 0 ) {
+        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Virtual disk %s size is
0!\n", file_name);
+        return 0;
+    }
+
+    return 1;
+}
+
 int libxl_device_disk_add(libxl_ctx *ctx, uint32_t domid,
libxl_device_disk *disk)
 {
     libxl__gc gc = LIBXL_INIT_GC(ctx);
@@ -835,6 +870,9 @@ int libxl_device_disk_add(libxl_ctx *ctx
     int devid;
     libxl__device device;
     int major, minor, rc;
+
+    if ( validate_virtual_disk(ctx, disk->physpath, disk->phystype) == 0 )
+        return ERROR_FAIL;

     front = flexarray_make(16, 1);
     if (!front) {

Re: [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file

[Gianni Tedesco]

On Wed, 2011-01-19 at 18:26 +0000, Kamala Narasimhan wrote:
> Apologies.  I inadvertently neglected Gianni's suggestion to switch to
> logging from fprintf.
> 
> Signed-off-by: Kamala Narasimhan <kamala.narasimhan@citrix.com>
> 
> Kamala
> 
> diff -r fe8a177ae9cb tools/libxl/libxl.c
> --- a/tools/libxl/libxl.c       Wed Jan 19 15:29:04 2011 +0000
> +++ b/tools/libxl/libxl.c       Wed Jan 19 13:23:16 2011 -0500
> @@ -826,6 +826,41 @@ skip_autopass:
> 
>  /******************************************************************************/
> 
> +static int validate_virtual_disk(libxl_ctx *ctx, char *file_name,
> libxl_disk_phystype disk_type)
> +{
> +    struct stat stat_buf;
> +
> +    if ( file_name == NULL ) {
> +        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Virtual disk file name is NULL!\n");
> +        return 0;
> +    }

I prefer assert() for things caused by programmer error. But in this
case we could just let the dereference below catch it...

> +    /* Return without further validation for empty cdrom drive.
> +       Note: Post 4.1 we need to change the interface to handle empty
> +       cdrom rather than go with the below assumption.
> +     */

So this handles CD-ROM images too? See below...

> +    if ( (strncmp(file_name, "", sizeof("")) == 0) && (disk_type == PHYSTYPE_PHY) )
> +        return 1;

Hrm, strlen(file_name) == 0 ? :)

> +
> +    if ( stat(file_name, &stat_buf) != 0 ) {
> +        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Stat on virtual disk %s returned error - \"%s\".\n",
> +            file_name, strerror(errno));
> +        return 0;
> +    }
> +    if ( disk_type == PHYSTYPE_PHY ) {
> +        if ( !(S_ISBLK(stat_buf.st_mode)) ) {
> +            LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Virtual disk %s is not a block device!\n",
> +                file_name);
> +            return 0;
> +        }
> +    } else if ( stat_buf.st_size == 0 ) {
> +        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Virtual disk %s size is 0!\n", file_name);
> +        return 0;
> +    }
> +
> +    return 1;
> +}
> +
>  int libxl_device_disk_add(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk)
>  {
>      libxl__gc gc = LIBXL_INIT_GC(ctx);
> @@ -835,6 +870,9 @@ int libxl_device_disk_add(libxl_ctx *ctx
>      int devid;
>      libxl__device device;
>      int major, minor, rc;
> +
> +    if ( validate_virtual_disk(ctx, disk->physpath, disk->phystype) == 0 )
> +        return ERROR_FAIL;
> 
>      front = flexarray_make(16, 1);
>      if (!front) {

In which case libxl_cdrom_insert() needs the same addition?

I also wonder about the motivation of the patch. To be honest, usually,
the best way to validate things like this is to go ahead and try to use
them and bail with an error at that time. Where do things bail out for
you and what problems does that cause? I also think that these checks
are maximal as well as minimal in that if we checked much further we
would be re-implementing code?

Gianni

Re: [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file

[Gianni Tedesco]

On Thu, 2011-01-20 at 14:04 +0000, Gianni Tedesco wrote:
> On Wed, 2011-01-19 at 18:26 +0000, Kamala Narasimhan wrote:
> > Apologies.  I inadvertently neglected Gianni's suggestion to switch to
> > logging from fprintf.
> > 
> > Signed-off-by: Kamala Narasimhan <kamala.narasimhan@citrix.com>
> > 
> > Kamala
> > 
> > diff -r fe8a177ae9cb tools/libxl/libxl.c
> > --- a/tools/libxl/libxl.c       Wed Jan 19 15:29:04 2011 +0000
> > +++ b/tools/libxl/libxl.c       Wed Jan 19 13:23:16 2011 -0500
> > @@ -826,6 +826,41 @@ skip_autopass:
> > 
> >  /******************************************************************************/
> > 
> > +static int validate_virtual_disk(libxl_ctx *ctx, char *file_name,
> > libxl_disk_phystype disk_type)
> > +{
> > +    struct stat stat_buf;
> > +
> > +    if ( file_name == NULL ) {
> > +        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Virtual disk file name is NULL!\n");
> > +        return 0;
> > +    }
> 
> I prefer assert() for things caused by programmer error. But in this
> case we could just let the dereference below catch it...
> 
> > +    /* Return without further validation for empty cdrom drive.
> > +       Note: Post 4.1 we need to change the interface to handle empty
> > +       cdrom rather than go with the below assumption.
> > +     */
> 
> So this handles CD-ROM images too? See below...
> In which case libxl_cdrom_insert() needs the same addition?

Ah, my mistake, libxl_cdrom_insert() is implemented in terms of
libxl_device_disk_add() so only one check is necessary.

Gianni

Re: [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file

[Kamala Narasimhan]

>
> So this handles CD-ROM images too? See below...
>

I didn't think CD-ROM image would be of type PHYSTYPE_PHY.

>> +    if ( (strncmp(file_name, "", sizeof("")) == 0) && (disk_type == PHYSTYPE_PHY) )
>> +        return 1;
>
> Hrm, strlen(file_name) == 0 ? :)
>

Of course :) I had a bug in that code earlier with a space between
quotes (" ") and I simply pulled out the space and replaced a bug with
stupidity :)

>
> In which case libxl_cdrom_insert() needs the same addition?
>
I think you answered this in a follow up email.

> I also wonder about the motivation of the patch. To be honest, usually,
> the best way to validate things like this is to go ahead and try to use
> them and bail with an error at that time. Where do things bail out for
> you and what problems does that cause? I also think that these checks
> are maximal as well as minimal in that if we checked much further we
> would be re-implementing code?

We fail in a very non obvious way when an invalid image path or zero
sized image is passed.  Not only do we perform a whole load of
initialization but also end up wasting time troubleshooting otherwise
trivial issues.  We shouldn't go all the way to spawn qemu and get to
its block device initialization code and then fail in a way that is
not obvious!  Although, once we establish a good communication between
upstream qemu and xl, this validation should go into upstream qemu as
they too would need to perform this kind of validation when run
independently (i.e. without an accelerator in their terminology).

Kamala

Re: [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file

[Gianni Tedesco]

On Thu, 2011-01-20 at 15:08 +0000, Kamala Narasimhan wrote:
> >
> > So this handles CD-ROM images too? See below...
> >
> 
> I didn't think CD-ROM image would be of type PHYSTYPE_PHY.
> 
> >> +    if ( (strncmp(file_name, "", sizeof("")) == 0) && (disk_type == PHYSTYPE_PHY) )
> >> +        return 1;
> >
> > Hrm, strlen(file_name) == 0 ? :)
> >
> 
> Of course :) I had a bug in that code earlier with a space between
> quotes (" ") and I simply pulled out the space and replaced a bug with
> stupidity :)

Happens to the best of us.

> >
> > In which case libxl_cdrom_insert() needs the same addition?
> >
> I think you answered this in a follow up email.
> 
> > I also wonder about the motivation of the patch. To be honest, usually,
> > the best way to validate things like this is to go ahead and try to use
> > them and bail with an error at that time. Where do things bail out for
> > you and what problems does that cause? I also think that these checks
> > are maximal as well as minimal in that if we checked much further we
> > would be re-implementing code?
> 
> We fail in a very non obvious way when an invalid image path or zero
> sized image is passed.  Not only do we perform a whole load of
> initialization but also end up wasting time troubleshooting otherwise
> trivial issues.  We shouldn't go all the way to spawn qemu and get to
> its block device initialization code and then fail in a way that is
> not obvious!  Although, once we establish a good communication between
> upstream qemu and xl, this validation should go into upstream qemu as
> they too would need to perform this kind of validation when run
> independently (i.e. without an accelerator in their terminology).

Hmm, yeah I can see how a zero length path could barf qemu command-line
parsing etc. It is also true that communication back and forth between
device model definitely needs improving. I think I agree with you about
the principle behind this patch.

Gianni

Re: [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file

[Kamala Narasimhan]

>>
>> So this handles CD-ROM images too? See below...
>>
>
> I didn't think CD-ROM image would be of type PHYSTYPE_PHY.

I see your point.  You mean the case where you specific cd-rom image
type with no cd-rom image file?  is that even supported?  How would
you specify it in the config file?

Kamala

Re: [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file

[Kamala Narasimhan]

Revised patch attached.  Please let me know if I missed a suggestion
or if you have new ones.

Signed-off-by: Kamala Narasimhan <kamala.narasimhan@citrix.com>

Kamala

diff -r fe8a177ae9cb tools/libxl/libxl.c
--- a/tools/libxl/libxl.c       Wed Jan 19 15:29:04 2011 +0000
+++ b/tools/libxl/libxl.c       Thu Jan 20 09:37:51 2011 -0500
@@ -826,6 +826,41 @@ skip_autopass:

 /******************************************************************************/

+static int validate_virtual_disk(libxl_ctx *ctx, char *file_name,
libxl_disk_phystype disk_type)
+{
+    struct stat stat_buf;
+
+    if ( file_name == NULL ) {
+        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Virtual disk file name is NULL!\n");
+        return 0;
+    }
+
+    /* Return without further validation for empty cdrom drive.
+       Note: Post 4.1 we need to change the interface to handle empty
+       cdrom rather than go with the below assumption.
+     */
+    if ( (strlen(file_name) == 0) && (disk_type == PHYSTYPE_PHY) )
+        return 1;
+
+    if ( stat(file_name, &stat_buf) != 0 ) {
+        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Stat on virtual disk %s
returned error - \"%s\".\n",
+            file_name, strerror(errno));
+        return 0;
+    }
+    if ( disk_type == PHYSTYPE_PHY ) {
+        if ( !(S_ISBLK(stat_buf.st_mode)) ) {
+            LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Virtual disk %s is not
a block device!\n",
+                file_name);
+            return 0;
+        }
+    } else if ( stat_buf.st_size == 0 ) {
+        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Virtual disk %s size is
0!\n", file_name);
+        return 0;
+    }
+
+    return 1;
+}
+
 int libxl_device_disk_add(libxl_ctx *ctx, uint32_t domid,
libxl_device_disk *disk)
 {
     libxl__gc gc = LIBXL_INIT_GC(ctx);
@@ -835,6 +870,9 @@ int libxl_device_disk_add(libxl_ctx *ctx
     int devid;
     libxl__device device;
     int major, minor, rc;
+
+    if ( validate_virtual_disk(ctx, disk->physpath, disk->phystype) == 0 )
+        return ERROR_FAIL;

     front = flexarray_make(16, 1);
     if (!front) {

Re: [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file

[Ian Jackson]

Gianni Tedesco writes ("Re: [Xen-devel] [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file"):
> On Wed, 2011-01-19 at 18:26 +0000, Kamala Narasimhan wrote:
> > +static int validate_virtual_disk(libxl_ctx *ctx, char *file_name,
> > libxl_disk_phystype disk_type)
> > +{
> > +    struct stat stat_buf;
> > +
> > +    if ( file_name == NULL ) {
> > +        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Virtual disk file name is NULL!\n");
> > +        return 0;
> > +    }
> 
> I prefer assert() for things caused by programmer error. But in this
> case we could just let the dereference below catch it...

Yes, quite.  I don't think this check should be here at all.

> > +    if ( (strncmp(file_name, "", sizeof("")) == 0) && (disk_type == PHYSTYPE_PHY) )
> > +        return 1;
> 
> Hrm, strlen(file_name) == 0 ? :)

Yes, that code is a bit too close to Daily WTF for my liking.

Also, convention in libxl is for functions to return error values, not
booleans.  I think that validate_virtual_disk should return 0 or
ERROR_INVAL, and libxl_device_disk add should simply pass on the
return code.

> >  int libxl_device_disk_add(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk)
> >  {
> >      libxl__gc gc = LIBXL_INIT_GC(ctx);
> > @@ -835,6 +870,9 @@ int libxl_device_disk_add(libxl_ctx *ctx
> >      int devid;
> >      libxl__device device;
> >      int major, minor, rc;
> > +
> > +    if ( validate_virtual_disk(ctx, disk->physpath, disk->phystype) == 0 )
> > +        return ERROR_FAIL;
> > 
> >      front = flexarray_make(16, 1);
> >      if (!front) {
> 
> In which case libxl_cdrom_insert() needs the same addition?

I think so.

> I also wonder about the motivation of the patch. To be honest, usually,
> the best way to validate things like this is to go ahead and try to use
> them and bail with an error at that time. Where do things bail out for
> you and what problems does that cause? I also think that these checks
> are maximal as well as minimal in that if we checked much further we
> would be re-implementing code?

I agree with this sentiment but currently the error handling is that
qemu-dm leaves a message in an obscure logfile.  We need to do
something better for 4.1 and writing new arrangments for plumbing
errors through is too late now.

For 4.2 we should revisit this and probably revert this patch and
replace it with something much better.

Ian.

Re: [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file

[Kamala Narasimhan]

>> I prefer assert() for things caused by programmer error. But in this
>> case we could just let the dereference below catch it...
>
> Yes, quite.  I don't think this check should be here at all.
>

I agree.  I missed this one.  Will send a revised patch.

>> > +    if ( (strncmp(file_name, "", sizeof("")) == 0) && (disk_type == PHYSTYPE_PHY) )
>> > +        return 1;
>>
>> Hrm, strlen(file_name) == 0 ? :)
>
> Yes, that code is a bit too close to Daily WTF for my liking.
>

Yeah, yeah :)   I already explained my stupidity behind this once:)

> Also, convention in libxl is for functions to return error values, not
> booleans.  I think that validate_virtual_disk should return 0 or
> ERROR_INVAL, and libxl_device_disk add should simply pass on the
> return code.
>

Sorry, I did add to the list of places we flout that convention in
libxl.  I will make the necessary changes.

>> In which case libxl_cdrom_insert() needs the same addition?
>
> I think so.
>

Not really, please see Gianni's follow up note.

Kamala

Re: [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file

[Kamala Narasimhan]

Here is a revised patch.  Please let me know if there are further suggestions.

Signed-off-by: Kamala Narasimhan <kamala.narasimhan@citrix.com>

Kamala

diff -r fe8a177ae9cb tools/libxl/libxl.c
--- a/tools/libxl/libxl.c       Wed Jan 19 15:29:04 2011 +0000
+++ b/tools/libxl/libxl.c       Thu Jan 20 16:09:42 2011 -0500
@@ -826,6 +826,38 @@ skip_autopass:

 /******************************************************************************/

+static int validate_virtual_disk(libxl_ctx *ctx, char *file_name,
libxl_disk_phystype disk_type)
+{
+    struct stat stat_buf;
+
+    assert(file_name);
+
+    /* Return without further validation for empty cdrom drive.
+       Note: Post 4.1 we need to change the interface to handle empty
+       cdrom rather than go with the below assumption.
+     */
+    if ( (strlen(file_name) == 0) && (disk_type == PHYSTYPE_PHY) )
+        return 0;
+
+    if ( stat(file_name, &stat_buf) != 0 ) {
+        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Stat on virtual disk %s
returned error - \"%s\".\n",
+            file_name, strerror(errno));
+        return ERROR_INVAL;
+    }
+    if ( disk_type == PHYSTYPE_PHY ) {
+        if ( !(S_ISBLK(stat_buf.st_mode)) ) {
+            LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Virtual disk %s is not
a block device!\n",
+                file_name);
+            return ERROR_INVAL;
+        }
+    } else if ( stat_buf.st_size == 0 ) {
+        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Virtual disk %s size is
0!\n", file_name);
+        return ERROR_INVAL;
+    }
+
+    return 0;
+}
+
 int libxl_device_disk_add(libxl_ctx *ctx, uint32_t domid,
libxl_device_disk *disk)
 {
     libxl__gc gc = LIBXL_INIT_GC(ctx);
@@ -835,6 +867,10 @@ int libxl_device_disk_add(libxl_ctx *ctx
     int devid;
     libxl__device device;
     int major, minor, rc;
+
+    rc = validate_virtual_disk(ctx, disk->physpath, disk->phystype);
+    if ( rc != 0 )
+        return rc;

     front = flexarray_make(16, 1);
     if (!front) {

Re: [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file

[Ian Jackson]

Kamala Narasimhan writes ("Re: [Xen-devel] [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file"):
> Here is a revised patch.  Please let me know if there are further suggestions.
...
> +    assert(file_name);

I don't think we need this.  If the pointer is null dereferencing it
will crash cleanly in just a moment.

> +    if ( (strlen(file_name) == 0) && (disk_type == PHYSTYPE_PHY) )
> +        return 0;

strlen still seems overkill.

> +    if ( stat(file_name, &stat_buf) != 0 ) {
> +        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Stat on virtual disk %s
> returned error - \"%s\".\n",

It is not conventional to capitalise the names of syscalls (or other
case-sensitive identifiers) even in messages, so "stat".

> +            file_name, strerror(errno));

Don't mess about with strerror yourself; use LIBXL__LOG_ERRNO.

> +    rc = validate_virtual_disk(ctx, disk->physpath, disk->phystype);
> +    if ( rc != 0 )
> +        return rc;

Convention in libxl is to write
    if (rc)
        return rc;

See for example libxl__fill_dom0_memory_info, which has
    if (rc)
        goto out;

Ian.

Re: [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file

[Gianni Tedesco]

On Fri, 2011-01-21 at 12:17 +0000, Ian Jackson wrote:
> Kamala Narasimhan writes ("Re: [Xen-devel] [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file"):
> > Here is a revised patch.  Please let me know if there are further suggestions.
> ...
> > +    assert(file_name);
> 
> I don't think we need this.  If the pointer is null dereferencing it
> will crash cleanly in just a moment.
> 
> > +    if ( (strlen(file_name) == 0) && (disk_type == PHYSTYPE_PHY) )
> > +        return 0;
> 
> strlen still seems overkill.

As opposed to file_name[0] == '\0' ?

I think strlen() is clearer about the meaning of the condition being
checked. Please let's not micro-optimise to this level.

Gianni

Re: [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file

[Kamala Narasimhan]

Ian - Apologies for the delay.  I think I have covered all comments so far.  If there are more I will get to it ASAP.  Please let me know.

Also, I switched email client to avoid word wrapping and other issues.  If you still find the format of the patches inconvenient, please let me know.

Signed-off-by: Kamala Narasimhan <kamala.narasimhan@citrix.com>

Kamala


diff -r fe8a177ae9cb tools/libxl/libxl.c
--- a/tools/libxl/libxl.c       Wed Jan 19 15:29:04 2011 +0000
+++ b/tools/libxl/libxl.c       Fri Jan 21 18:00:37 2011 -0500
@@ -826,6 +826,35 @@ skip_autopass:

 /******************************************************************************/

+static int validate_virtual_disk(libxl_ctx *ctx, char *file_name, libxl_disk_phystype disk_type)
+{
+    struct stat stat_buf;
+
+    /* Return without further validation for empty cdrom drive.
+       Note: Post 4.1 we need to change the interface to handle empty
+       cdrom rather than go with the below assumption.
+     */
+    if ( (file_name[0] == '\0') && (disk_type == PHYSTYPE_PHY) )
+        return 0;
+
+    if ( stat(file_name, &stat_buf) != 0 ) {
+        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "failed to stat %s", file_name);
+        return ERROR_INVAL;
+    }
+    if ( disk_type == PHYSTYPE_PHY ) {
+        if ( !(S_ISBLK(stat_buf.st_mode)) ) {
+            LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Virtual disk %s is not a block device!\n",
+                file_name);
+            return ERROR_INVAL;
+        }
+    } else if ( stat_buf.st_size == 0 ) {
+        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Virtual disk %s size is 0!\n", file_name);
+        return ERROR_INVAL;
+    }
+
+    return 0;
+}
+
 int libxl_device_disk_add(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk)
 {
     libxl__gc gc = LIBXL_INIT_GC(ctx);
@@ -835,6 +864,10 @@ int libxl_device_disk_add(libxl_ctx *ctx
     int devid;
     libxl__device device;
     int major, minor, rc;
+
+    rc = validate_virtual_disk(ctx, disk->physpath, disk->phystype);
+    if (rc)
+        return rc;

     front = flexarray_make(16, 1);
     if (!front) {

Re: [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file

[Kamala Narasimhan]

I am resending this with tweaks though I am not sure if you plan to accept it for 4.1.  Hopefully there is no word wrapping issues etc.

Signed-off-by: Kamala Narasimhan <kamala.narasimhan@citrix.com>

Kamala


diff -r fe8a177ae9cb tools/libxl/libxl.c
--- a/tools/libxl/libxl.c       Wed Jan 19 15:29:04 2011 +0000
+++ b/tools/libxl/libxl.c       Sun Jan 23 12:42:21 2011 -0500
@@ -40,11 +40,19 @@

 int libxl_ctx_init(libxl_ctx *ctx, int version, xentoollog_logger *lg)
 {
+    struct stat stat_buf;
+
     if (version != LIBXL_VERSION)
         return ERROR_VERSION;
     memset(ctx, 0, sizeof(libxl_ctx));
     ctx->lg = lg;
     memset(&ctx->version_info, 0, sizeof(libxl_version_info));
+
+    if ( stat(XENSTORE_PID_FILE, &stat_buf) != 0 ) {
+        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Is xenstore daemon running?\n"
+                     "failed to stat %s", XENSTORE_PID_FILE);
+        return ERROR_FAIL;
+    }

     ctx->xch = xc_interface_open(lg,lg,0);
     if (!ctx->xch) {
diff -r fe8a177ae9cb tools/libxl/libxl_internal.h
--- a/tools/libxl/libxl_internal.h      Wed Jan 19 15:29:04 2011 +0000
+++ b/tools/libxl/libxl_internal.h      Sun Jan 23 12:42:21 2011 -0500
@@ -104,6 +104,7 @@ typedef struct {
 #define AUTO_PHP_SLOT          0x100
 #define SYSFS_PCI_DEV          "/sys/bus/pci/devices"
 #define SYSFS_PCIBACK_DRIVER   "/sys/bus/pci/drivers/pciback"
+#define XENSTORE_PID_FILE      "/var/run/xenstore.pid"

 #define PROC_PCI_NUM_RESOURCES 7
 #define PCI_BAR_IO             0x01

Re: [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file

[Kamala Narasimhan]

BTW, the below patch is for checking dependencies in xl.  I should have responded to "[Xen-devel] [PATCH] xl: Check for dependencies in xl" thread instead.  I already sent the disk validation patch.

Kamala

Kamala Narasimhan wrote:
> I am resending this with tweaks though I am not sure if you plan to accept it for 4.1.  Hopefully there is no word wrapping issues etc.
> 
> Signed-off-by: Kamala Narasimhan <kamala.narasimhan@citrix.com>
> 
> Kamala
> 
> 
> diff -r fe8a177ae9cb tools/libxl/libxl.c
> --- a/tools/libxl/libxl.c       Wed Jan 19 15:29:04 2011 +0000
> +++ b/tools/libxl/libxl.c       Sun Jan 23 12:42:21 2011 -0500
> @@ -40,11 +40,19 @@
> 
>  int libxl_ctx_init(libxl_ctx *ctx, int version, xentoollog_logger *lg)
>  {
> +    struct stat stat_buf;
> +
>      if (version != LIBXL_VERSION)
>          return ERROR_VERSION;
>      memset(ctx, 0, sizeof(libxl_ctx));
>      ctx->lg = lg;
>      memset(&ctx->version_info, 0, sizeof(libxl_version_info));
> +
> +    if ( stat(XENSTORE_PID_FILE, &stat_buf) != 0 ) {
> +        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Is xenstore daemon running?\n"
> +                     "failed to stat %s", XENSTORE_PID_FILE);
> +        return ERROR_FAIL;
> +    }
> 
>      ctx->xch = xc_interface_open(lg,lg,0);
>      if (!ctx->xch) {
> diff -r fe8a177ae9cb tools/libxl/libxl_internal.h
> --- a/tools/libxl/libxl_internal.h      Wed Jan 19 15:29:04 2011 +0000
> +++ b/tools/libxl/libxl_internal.h      Sun Jan 23 12:42:21 2011 -0500
> @@ -104,6 +104,7 @@ typedef struct {
>  #define AUTO_PHP_SLOT          0x100
>  #define SYSFS_PCI_DEV          "/sys/bus/pci/devices"
>  #define SYSFS_PCIBACK_DRIVER   "/sys/bus/pci/drivers/pciback"
> +#define XENSTORE_PID_FILE      "/var/run/xenstore.pid"
> 
>  #define PROC_PCI_NUM_RESOURCES 7
>  #define PCI_BAR_IO             0x01
> 
> 
> 

Re: [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file

[Ian Jackson]

Kamala Narasimhan writes ("Re: [Xen-devel] [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file"):
> Ian - Apologies for the delay.  I think I have covered all comments so far.  If there are more I will get to it ASAP.  Please let me know.

Thanks, I have applied your patch, with a minor tweak to make it work
with Stefano's PHYSTYPE_EMPTY patch.

Ian.

Re: [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file

[Kamala Narasimhan]

On Tue, Jan 25, 2011 at 1:10 PM, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> Thanks, I have applied your patch, with a minor tweak to make it work
> with Stefano's PHYSTYPE_EMPTY patch.
>
Ian - I am afraid the validation in its current form is going to fail
for vhds with prefix 'tap:aio:vhd:' as I just found out.  I will send
a patch to fix that.  In the mean time, if there is a wiki/doc that
lists the different formats that can be passed through the disk
option, I can verify all of those so as not to introduce any further
regression.

Kamala

Re: [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file

[Ian Campbell]

On Tue, 2011-01-25 at 18:10 +0000, Ian Jackson wrote:
> Kamala Narasimhan writes ("Re: [Xen-devel] [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file"):
> > Ian - Apologies for the delay.  I think I have covered all comments so far.  If there are more I will get to it ASAP.  Please let me know.
> 
> Thanks, I have applied your patch, with a minor tweak to make it work
> with Stefano's PHYSTYPE_EMPTY patch.

With this patch applied I get an error using an LVM device via file://
(which is currently necessary on upstream dom0 support in order to
activate the qdisk support):
        # xl cr -c /etc/xen/debian-x86_32p-1
        Parsing config file /etc/xen/debian-x86_32p-1
        libxl: error: libxl.c:854:validate_virtual_disk Virtual disk /dev/VG/debian-x86_32-1 size is 0!
        
My disk stanza is
        # grep ^disk /etc/xen/debian-x86_32p-1
        disk = ['file:/dev/VG/debian-x86_32-1,xvda,w']

I think the issue is in this fragment:
    if ( disk_type == PHYSTYPE_PHY ) {
        if ( !(S_ISBLK(stat_buf.st_mode)) ) {
            LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Virtual disk %s is not a block device!\n",
                file_name);
            return ERROR_INVAL;
        }
    } else if ( stat_buf.st_size == 0 ) {
        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Virtual disk %s size is 0!\n", file_name);
        return ERROR_INVAL;
    }
since stat(2) says that st_size is only valid for a regular file or
symbolic link (the latter being irrelevant in this case since we are
using stat() and not lstat()).

Ian.

8<--------------------


# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1296037362 0
# Node ID e4e69622dc95037eab6740f79ecf9c1d05bca529
# Parent  751232406cedb808149ece09480f7a7ec552483d
libxl: only check size of regular files when validating a virtual disk

st_size is only valid for regular files and not block devices.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

diff -r 751232406ced -r e4e69622dc95 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Wed Jan 26 09:11:34 2011 +0000
+++ b/tools/libxl/libxl.c	Wed Jan 26 10:22:42 2011 +0000
@@ -850,7 +850,7 @@ static int validate_virtual_disk(libxl_c
                 file_name);
             return ERROR_INVAL;
         }
-    } else if ( stat_buf.st_size == 0 ) {
+    } else if ( S_ISREG(stat_buf.st_mode) && stat_buf.st_size == 0 ) {
         LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Virtual disk %s size is 0!\n", file_name);
         return ERROR_INVAL;
     }

Re: [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file

[Ian Jackson]

Kamala Narasimhan writes ("Re: [Xen-devel] [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file"):
> Ian - I am afraid the validation in its current form is going to fail
> for vhds with prefix 'tap:aio:vhd:' as I just found out.  I will send
> a patch to fix that.  In the mean time, if there is a wiki/doc that
> lists the different formats that can be passed through the disk
> option, I can verify all of those so as not to introduce any further
> regression.

Thanks.  Many of these strings are quite arcane!

Ian.

Re: [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file"):
> With this patch applied I get an error using an LVM device via file://
> (which is currently necessary on upstream dom0 support in order to
> activate the qdisk support):

Hrm.  I think the toolstack should really not depend on the user to
write "file:" some of the time and "phy:" the rest of the time
according to arcane rules.

But it's probably too late to fix this properly for 4.1.  In 4.2 xl
should parse "phy:" and "file:" to PHYSTYPE_PLAIN or something and
then libxl decide for itself what to do based on backend we are going
to use and whether the object is a block device.

So I have applied your patch, thanks.

Ian.

Re: [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file

[Ian Campbell]

On Wed, 2011-01-26 at 11:48 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [Xen-devel] [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file"):
> > With this patch applied I get an error using an LVM device via file://
> > (which is currently necessary on upstream dom0 support in order to
> > activate the qdisk support):
> 
> Hrm.  I think the toolstack should really not depend on the user to
> write "file:" some of the time and "phy:" the rest of the time
> according to arcane rules.
> 
> But it's probably too late to fix this properly for 4.1.  In 4.2 xl
> should parse "phy:" and "file:" to PHYSTYPE_PLAIN or something and
> then libxl decide for itself what to do based on backend we are going
> to use and whether the object is a block device.

I think the original rationale for the need to do this was the lack of a
good way to tell if blkback is available on the current host. phy://
becomes blkback whereas file:// becomes blktap whose absence we detect
and fallback to qdisk.

I agree that we should try and do better for 4.2 though.

Ian.

Re: [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file

[Kamala Narasimhan]

Here is a patch that special cases tap/aio during validation.

I am not taking into account qcow and qcow2 as I hear they are broken with tap and shouldn't be used for that reason.

Signed-off-by: Kamala Narasimhan <kamala.narasimhan@citrix.com>

Kamala

diff -r 67d5b8004947 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c       Wed Jan 26 11:58:45 2011 +0000
+++ b/tools/libxl/libxl.c       Wed Jan 26 12:07:54 2011 -0500
@@ -836,22 +836,26 @@ static int validate_virtual_disk(libxl_c
 static int validate_virtual_disk(libxl_ctx *ctx, char *file_name, libxl_disk_phystype disk_type)
 {
     struct stat stat_buf;
+    int count = 0;

     if ( (file_name[0] == '\0') && (disk_type == PHYSTYPE_EMPTY) )
         return 0;

-    if ( stat(file_name, &stat_buf) != 0 ) {
-        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "failed to stat %s", file_name);
+    if ( disk_type == PHYSTYPE_AIO )
+        count = strlen("vhd:");
+
+    if ( stat(&file_name[count], &stat_buf) != 0 ) {
+        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "failed to stat %s", &file_name[count]);
         return ERROR_INVAL;
     }
     if ( disk_type == PHYSTYPE_PHY ) {
         if ( !(S_ISBLK(stat_buf.st_mode)) ) {
             LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Virtual disk %s is not a block device!\n",
-                file_name);
+                &file_name[count]);
             return ERROR_INVAL;
         }
     } else if ( S_ISREG(stat_buf.st_mode) && stat_buf.st_size == 0 ) {
-        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Virtual disk %s size is 0!\n", file_name);
+        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Virtual disk %s size is 0!\n", &file_name[count]);
         return ERROR_INVAL;
     }