From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Jackson Subject: Re: [PATCH v3 5/7] libxl: introduce libxl__alloc_vdev Date: Fri, 20 Apr 2012 15:28:59 +0100 Message-ID: <20369.29355.670145.159299@mariner.uk.xensource.com> References: <1334601190-14187-5-git-send-email-stefano.stabellini@eu.citrix.com> <20365.44544.739441.639648@mariner.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Stefano Stabellini Cc: "xen-devel@lists.xensource.com" , Ian Campbell List-Id: xen-devel@lists.xenproject.org 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. > > > +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...) > > > 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. Ian.