Re: [Xen-devel] [PATCH v3 5/7] libxl: introduce libxl__alloc_vdev

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

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.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel