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

[Kamala Narasimhan <kamala.narasimhan@xxxxxxxxx>]

>
> 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@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel