WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

Re: [Xen-devel] [PATCH] xl: Perform minimal validation of virtual disk f

To: Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
Subject: Re: [Xen-devel] [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file
From: Kamala Narasimhan <kamala.narasimhan@xxxxxxxxx>
Date: Thu, 20 Jan 2011 11:46:24 -0500
Cc: Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>, Gianni Tedesco <gianni.tedesco@xxxxxxxxxx>, Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
Delivery-date: Thu, 20 Jan 2011 08:47:33 -0800
Dkim-signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type :content-transfer-encoding; bh=ymtKq+TWD1MGnYgrkUJ+Sqs4LFyuLsRJljUSHVn5qs0=; b=op20/b8b5L6/0DOq3ZT+9nbufyxl0ZZtbnVfQAHp7Pu5y3IqHXvuz1yayCP714ZcrH SctuanERm2qhXPhwFcmWH8o9jGDreyoy3FFrQCPb+DU/iWrz5jrULBfLNT2VFe8uHSn5 J1N19OY7LHJlBlDMXcFELC/Ez/cIY2SkdHF0Y=
Domainkey-signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:content-transfer-encoding; b=Iy5cfUuM5VoMNxvYwGwdkaxnwdo/ge6ToMbvfl56vSthRdcvXxzCUuGnIzVKrxIsMo oZYPh7/GJidBXWxlc/FFzsGIdDT7WXr6BNZ6fB1LCbrSXw5Izc74FNjjTKuldD2wtNzX Hh6KpRPbHmvsgS26x64cX1uqPsTr1J3AaP0V8=
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <19768.22912.878633.622270@xxxxxxxxxxxxxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <AANLkTimSUym0u+SNm0AvNp3AZQQFspetAaXmNShkPJd4@xxxxxxxxxxxxxx> <1294995912.8240.86.camel@xxxxxxxxxxxxxxxxxxxxxx> <AANLkTikfUHHc+-gVkgnRJc722wObLF3TumpK5WSfJVAE@xxxxxxxxxxxxxx> <1295024348.12018.222.camel@xxxxxxxxxxxxxxxxxxxxxx> <AANLkTikf3TVJwE_N-OyuS-UhyA8+cgzAG__9hz3AETeM@xxxxxxxxxxxxxx> <AANLkTinJ=PYsC6vbPvU8g2T8NmyohLa=4rd9zfhTMCCO@xxxxxxxxxxxxxx> <AANLkTin1AGxH26158mn37_Oar1PgSSJoJOnGHs+XnxsV@xxxxxxxxxxxxxx> <1295532296.12018.337.camel@xxxxxxxxxxxxxxxxxxxxxx> <19768.22912.878633.622270@xxxxxxxxxxxxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
>> 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

<Prev in Thread] Current Thread [Next in Thread>