From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefano Stabellini Subject: Re: [PATCH v3 5/7] libxl: introduce libxl__alloc_vdev Date: Mon, 23 Apr 2012 18:40:37 +0100 Message-ID: References: <1334601190-14187-5-git-send-email-stefano.stabellini@eu.citrix.com> <20365.44544.739441.639648@mariner.uk.xensource.com> <20369.29355.670145.159299@mariner.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20369.29355.670145.159299@mariner.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Jackson Cc: "xen-devel@lists.xensource.com" , Ian Campbell , Stefano Stabellini List-Id: xen-devel@lists.xenproject.org On Fri, 20 Apr 2012, Ian Jackson wrote: > Stefano Stabellini writes ("Re: [Xen-devel] [PATCH v3 5/7] libxl: introduce libxl__alloc_vdev"): > > On Tue, 17 Apr 2012, Ian Jackson wrote: > > > Stefano Stabellini writes ("[Xen-devel] [PATCH v3 5/7] libxl: introduce libxl__alloc_vdev"): > > > > + devid = libxl__device_disk_dev_number(vdev, NULL, NULL); > > > > + if (libxl__xs_read(gc, t, > > > > + libxl__sprintf(gc, "%s/device/vbd/%d/backend", > > > > + dompath, devid)) == NULL) > > > > + return libxl__devid_to_vdev(gc, devid); > > > > > > What if the error is not ENOENT ? > > > > we should return NULL > > I don't think that's correct. If, say, the error is EACCES, then the > domain creation should be aborted with a message about that, since the > system has been installed incorrectly. > > Compare this situation with the recent pygrub failure, where > libfsimage+pygrub turned all errors of the form "something went wrong > loading this plugin" into "the kernel was not found"; so when a > completely empty .so was loaded as a plugin the result was not "OMG > WTF this is totally broken" but "sorry can't find your kernel in this > filesystem" (when really the problem is that pygrub+libfsimage knew > that the filesystem was one they were supposed to support but the > plugin for it was utterly broken). > > This reminds me of our other recent discussion about error handling, > of receiving unexpected toolstack migration info. In general any > unanticipated situation should be treated as a fatal error. Only > anticipated situations should result in the software continuing in a > degraded manner. NULL is an abort condition and libxl__device_disk_local_attach prints a useful message. > > > > +static char *encode_disk_name(char *ptr, unsigned int n) > > > > > > There is no clearly defined upper bound on the buffer space needed by > > > this function. > > > > I know but this function is used as is in Linux where the stack is > > even smaller. I'll add an upper bound anyway. > > At the very least a comment is needed to demonstrate that it's > correct, but a bound in the code would be better. (Also I'm surprised > that you chose a recursive rather than iterative implementation of a > what is a base conversion routine...) OK > > > > diff --git a/tools/libxl/libxl_netbsd.c b/tools/libxl/libxl_netbsd.c > > > > index 9e0ed6d..c8977ac 100644 > > > > --- a/tools/libxl/libxl_netbsd.c > > > > +++ b/tools/libxl/libxl_netbsd.c > > > ... > > > > +char *libxl__devid_to_vdev(libxl__gc *gc, int devid) > > > > +{ > > > > + /* TODO */ > > > > + return NULL; > > > > +} > > > > > > I guess this is going to be fixed in a future version of the patch ? > > > > I don't think so: I don't know anything about netbsd and local_attach > > doesn't work there anyway. > > What is the error behaviour if NULL is returned here ? I forget the > rest of the patch, but once again we should make sure that we abort if > this situation occurs. NULL is returned by libxl__alloc_vdev, then it is the same as before: eventually the domain creation terminates with an fatal error.