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

[Gianni Tedesco <gianni.tedesco@xxxxxxxxxx>]

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