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

[Kamala Narasimhan <kamala.narasimhan@xxxxxxxxx>]

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