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: Kamala Narasimhan <kamala.narasimhan@xxxxxxxxx>
Subject: Re: [Xen-devel] [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file
From: Gianni Tedesco <gianni.tedesco@xxxxxxxxxx>
Date: Thu, 20 Jan 2011 15:22:18 +0000
Cc: Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>, Stefano Stabellini <Stefano.Stabellini@xxxxxxxxxxxxx>
Delivery-date: Thu, 20 Jan 2011 07:23:13 -0800
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <AANLkTi=7oa7DZ22eXDfaMBWuLCe41LbN2xhpOuBPFsf1@xxxxxxxxxxxxxx>
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> <AANLkTi=7oa7DZ22eXDfaMBWuLCe41LbN2xhpOuBPFsf1@xxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
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

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