Kamala Narasimhan
2011-Jan-13 15:35 UTC
[Xen-devel] [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file
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 */
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Ian Campbell
2011-Jan-14 09:05 UTC
Re: [Xen-devel] [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file
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_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Kamala Narasimhan
2011-Jan-14 14:55 UTC
Re: [Xen-devel] [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Gianni Tedesco
2011-Jan-14 16:59 UTC
Re: [Xen-devel] [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Kamala Narasimhan
2011-Jan-14 17:17 UTC
Re: [Xen-devel] [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file
> 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Kamala Narasimhan
2011-Jan-19 18:09 UTC
Re: [Xen-devel] [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file
> 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) { _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Kamala Narasimhan
2011-Jan-19 18:26 UTC
Re: [Xen-devel] [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file
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) {
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Gianni Tedesco
2011-Jan-20 14:04 UTC
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:> 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Gianni Tedesco
2011-Jan-20 14:12 UTC
Re: [Xen-devel] [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Kamala Narasimhan
2011-Jan-20 15:08 UTC
Re: [Xen-devel] [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file
> > 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Gianni Tedesco
2011-Jan-20 15:22 UTC
Re: [Xen-devel] [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Kamala Narasimhan
2011-Jan-20 15:22 UTC
Re: [Xen-devel] [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file
>> >> 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Kamala Narasimhan
2011-Jan-20 15:41 UTC
Re: [Xen-devel] [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file
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) {
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Ian Jackson
2011-Jan-20 15:49 UTC
Re: [Xen-devel] [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file
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.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Kamala Narasimhan
2011-Jan-20 16:46 UTC
Re: [Xen-devel] [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file
>> 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Kamala Narasimhan
2011-Jan-20 21:14 UTC
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.
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) {
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Ian Jackson
2011-Jan-21 12:17 UTC
Re: [Xen-devel] [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file
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.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Gianni Tedesco
2011-Jan-21 13:27 UTC
Re: [Xen-devel] [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Kamala Narasimhan
2011-Jan-22 02:33 UTC
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.
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) {
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Kamala Narasimhan
2011-Jan-24 14:18 UTC
Re: [Xen-devel] [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Kamala Narasimhan
2011-Jan-24 14:31 UTC
Re: [Xen-devel] [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file
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 > > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Jackson
2011-Jan-25 18:10 UTC
Re: [Xen-devel] [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file
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.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Kamala Narasimhan
2011-Jan-26 03:07 UTC
Re: [Xen-devel] [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Jan-26 10:27 UTC
Re: [Xen-devel] [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file
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; } _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Jackson
2011-Jan-26 11:43 UTC
Re: [Xen-devel] [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file
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.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Ian Jackson
2011-Jan-26 11:48 UTC
Re: [Xen-devel] [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file
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.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Ian Campbell
2011-Jan-26 11:54 UTC
Re: [Xen-devel] [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file
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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Kamala Narasimhan
2011-Jan-26 18:02 UTC
Re: [Xen-devel] [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file
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;
}
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel