Re: [Xen-devel] [PATCH v5 6/8] libxl: introduce libxl__alloc_vdev

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Fri, 4 May 2012, Ian Jackson wrote:
> Stefano Stabellini writes ("[PATCH v5 6/8] libxl: introduce 
> libxl__alloc_vdev"):
> > Introduce libxl__alloc_vdev: find a spare virtual block device in the
> > domain passed as argument.
> ...
> > +    do {
> > +        vdev = GCSPRINTF("d%dp0", disk);
> > +        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) {
> > +            if (errno == ENOENT)
> > +                return libxl__devid_to_localdev(gc, devid);
> > +            else
> > +                return NULL;
> > +        }
> > +        disk++;
> > +    } while (disk <= (1 << 20));
> 
> This should be "<", as disk==(1<<20) is too big.
> And the magic number 20 should not be repeated.
> 
> But it turns out that you don't need to do this check here at all
> because libxl__device_disk_dev_number will check this, correctly.
> 
> All you need to do is actually pay attention to its error return.

OK

> > +    return NULL;
> 
> All the NULL error exits should log an error surely.

The error log is in the next patch. Should I add a second log here?
Or maybe I should move the error log from the caller to the callee?


> > +/* same as in Linux */
> > +static char *encode_disk_name(char *ptr, unsigned int n)
> > +{
> > +    if (n >= 26)
> > +        ptr = encode_disk_name(ptr, n / 26 - 1);
> > +    *ptr = 'a' + n % 26;
> > +    return ptr + 1;
> > +}
> 
> This still makes it difficult to see that the buffer cannot be
> overrun.  I mentioned this in response to a previous posting of this
> code and IIRC you were going to do something about it, if only a
> comment explaining what the maximum buffer length is and why it's
> safe.

I am keen on keeping the code as is, because it is the same that is in
Linux (see comment above).
I'll add a comment.

> > +    strcpy(ret, "xvd");
> > +    ptr = encode_disk_name(ret + 3, offset);
> > +    if (minor % nr_parts == 0)
> > +        *ptr = 0;
> > +    else
> > +        snprintf(ptr, ret + 32 - ptr,
> > +                "%d", minor & (nr_parts - 1));
> 
> You do not handle snprintf buffer truncation sensibly here.  As it
> happens I think this overflow cannot happen but this deserves a
> comment at the very least, to explain the lack of error handling.

Same here, I am keen on keeping the code as is, because it is the same
that is in Linux.
I'll add a comment.

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