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

[Ian Jackson <Ian.Jackson@eu.citrix.com>]

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.