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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Tue, 17 Apr 2012, Ian Jackson wrote:
> Stefano Stabellini writes ("[Xen-devel] [PATCH v3 5/7] libxl: introduce 
> libxl__alloc_vdev"):
> > +static char * libxl__alloc_vdev(libxl__gc *gc, uint32_t domid,
> > +        char *blkdev_start, xs_transaction_t t)
> > +{
> > +    int devid = 0;
> > +    char *vdev = libxl__strdup(gc, blkdev_start);
> > +    char *dompath = libxl__xs_get_dompath(gc, domid);
> > +
> > +    do {
> > +        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


> > +        vdev[strlen(vdev) - 1]++;
> > +    } while(vdev[strlen(vdev) - 1] <= 'z');
>               ^
> Missing whitespace.

OK

> > +_hidden char *libxl__devid_to_vdev(libxl__gc *gc, int devid);
> 
> I think the terminology here needs to be corrected.  "vdev" is the
> virtual device name supplied to libxl - a symbolic way of specifying a
> devid in a guest-independent way.
> 
> Whereas what we actually mean is the non-portable name in this guest
> OS for a device.  How about
>   libxl__devid_to_localdev
> ?

right, I think it would be a bit clearer

> 
> > +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.


> > 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.

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