From mboxrd@z Thu Jan 1 00:00:00 1970 From: Gianni Tedesco Subject: Re: [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file Date: Thu, 20 Jan 2011 15:22:18 +0000 Message-ID: <1295536938.12018.345.camel@qabil.uk.xensource.com> References: <1294995912.8240.86.camel@zakaz.uk.xensource.com> <1295024348.12018.222.camel@qabil.uk.xensource.com> <1295532296.12018.337.camel@qabil.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Kamala Narasimhan Cc: Ian Campbell , "xen-devel@lists.xensource.com" , Stefano Stabellini List-Id: xen-devel@lists.xenproject.org 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