[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] 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@xxxxxxxxxxxxxxxxxxx http://lists.xensource.com/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |