Re: [Xen-devel] [PATCH 03/18] xen: introduce 'xen-qdisk'

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Qemu-devel [mailto:qemu-devel-
> bounces+paul.durrant=citrix.com@xxxxxxxxxx] On Behalf Of Paul Durrant
> Sent: 04 December 2018 15:20
> To: Anthony Perard <anthony.perard@xxxxxxxxxx>
> Cc: Kevin Wolf <kwolf@xxxxxxxxxx>; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; qemu-block@xxxxxxxxxx; qemu-devel@xxxxxxxxxx;
> Max Reitz <mreitz@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [Qemu-devel] [PATCH 03/18] xen: introduce 'xen-qdisk'
> 
> > -----Original Message-----
> > From: Anthony PERARD [mailto:anthony.perard@xxxxxxxxxx]
> > Sent: 29 November 2018 16:06
> > To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> > Cc: qemu-block@xxxxxxxxxx; qemu-devel@xxxxxxxxxx; xen-
> > devel@xxxxxxxxxxxxxxxxxxxx; Kevin Wolf <kwolf@xxxxxxxxxx>; Max Reitz
> > <mreitz@xxxxxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > Subject: Re: [PATCH 03/18] xen: introduce 'xen-qdisk'
> >
> > On Wed, Nov 21, 2018 at 03:11:56PM +0000, Paul Durrant wrote:
> > > This patch adds a new XenDevice: 'xen-qdisk' [1]. This will eventually
> > > replace the 'xen_disk' legacy PV backend but it is illustrative to
> build
> > > up the implementation incrementally, along with the XenBus/XenDevice
> > > framework. Subsequent patches will therefore add to this device's
> > > implementation as new features are added to the framework.
> > >
> > > After this patch has been applied it is possible to instantiate a new
> > > 'xen-qdisk' device with a single 'vdev' parameter, which accepts
> values
> > > adhering to the Xen VBD naming scheme [2]. For example, a command-line
> > > instantiation of a xen-qdisk can be done with an argument similar to
> the
> > > following:
> > >
> > > -device xen-qdisk,vdev=hda
> >
> > That works when QEMU boot, but doing the same thing once the guest have
> > booted, via QMP, doesn't. Here is the result (tested in qmp-shell):
> >
> > (QEMU) device_add driver=xen-qdisk vdev=hda
> > {
> >     "error": {
> >         "class": "GenericError",
> >         "desc": "Bus 'xen-bus.0' does not support hotplugging"
> >     }
> > }
> >
> > That's probably why I've asked about the hotplug capability on the
> > previous patch.
> >
> 
> Ok. I've added the hotplug now so I'll make sure QMP DTRT.
> 
> > > The implementation of the vdev parameter formulates the appropriate
> VBD
> > > number for use in the PV protocol.
> > >
> > > [1] The name 'qdisk' as always been the name given to the QEMU
> > >     implementation of the Xen PV block protocol backend implementation
> > > [2] http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=docs/man/xen-
> vbd-
> > interface.markdown.7
> >
> > Maybe a link to the generated docs would be better:
> > https://xenbits.xen.org/docs/unstable/man/xen-vbd-interface.7.html
> >
> 
> Ok.
> 
> > Also, it would be useful to have the same link in the source code.
> >
> 
> Yes, I'll add a comment.
> 
> > > diff --git a/hw/block/xen-qdisk.c b/hw/block/xen-qdisk.c
> > > new file mode 100644
> > > index 0000000000..72122073f7
> > > --- /dev/null
> > > +++ b/hw/block/xen-qdisk.c
> > [...]
> > > +static char *disk_to_vbd_name(unsigned int disk)
> > > +{
> > > +    unsigned int len = DIV_ROUND_UP(disk, 26);
> > > +    char *name = g_malloc0(len + 1);
> > > +
> > > +    do {
> > > +        name[len--] = 'a' + (disk % 26);
> > > +        disk /= 26;
> > > +    } while (disk != 0);
> > > +    assert(len == 0);
> > > +
> > > +    return name;
> > > +}
> >
> > That function doesn't work.
> >
> > For a simple xvdp, (so disk==15), it return "", I mean "\0p".
> >
> > For a more complicated 'xvdbhwza', we have len == 22901. And the assert
> > failed.
> >
> > Maybe the recursing algo in libxl would be fine, with a buffer that is
> > big enough, and could probably be on the stack (in _get_vdev).
> 
> I used libxl__device_disk_dev_number() as my model (as well as cross-
> checking with the spec), but I guess a recursing algorithm would be
> neater.
> 
> >
> > > +
> > > +    switch (vdev->type) {
> > > +    case XEN_QDISK_VDEV_TYPE_DP:
> > > +    case XEN_QDISK_VDEV_TYPE_XVD:
> > > +        if (vdev->disk < (1 << 4) && vdev->partition < (1 << 4)) {
> > > +            vdev->number = (202 << 8) | (vdev->disk << 4) |
> > > +                vdev->partition;
> > > +        } else if (vdev->disk < (1 << 20) && vdev->partition < (1 <<
> > 8)) {
> > > +            vdev->number = (1 << 28) | (vdev->disk << 8) |
> > > +                vdev->partition;
> > > +        } else {
> > > +            goto invalid;
> > > +        }
> > > +        break;
> > > +
> > > +    case XEN_QDISK_VDEV_TYPE_HD:
> > > +        if ((vdev->disk == 0 || vdev->disk == 1) &&
> > > +            vdev->partition < (1 << 4)) {
> >
> > I think that should be:
> >
> >     vdev->partition < (1 << 6)
> >
> > Because hd disk have 0..63 partitions.
> 
> Yes, I must have typo-ed it...
> 
> >
> > > +            vdev->number = (3 << 8) | (vdev->disk << 6) | vdev-
> > >partition;
> > > +        } else if ((vdev->disk == 2 || vdev->disk == 3) &&
> > > +                   vdev->partition < (1 << 4)) {
> >
> > same here.
> 
> ...and then cut'n'pasted.
> 
> >
> > > +            vdev->number = (22 << 8) | ((vdev->disk - 2) << 6) |
> > > +                vdev->partition;
> > > +        } else {
> > > +            goto invalid;
> > > +        }
> > > +        break;
> > > +
> > > +    case XEN_QDISK_VDEV_TYPE_SD:
> > > +        if (vdev->disk < (1 << 4) && vdev->partition < (1 << 4)) {
> > > +            vdev->number = (8 << 8) | (vdev->disk << 4) | vdev-
> > >partition;
> > > +        } else {
> > > +            goto invalid;
> > > +        }
> > > +        break;
> > > +
> > > +    default:
> > > +        goto invalid;
> > > +    }
> > > +
> > > +    g_free(str);
> > > +    vdev->valid = true;
> > > +    return;
> > > +
> > > +invalid:
> > > +    error_setg(errp, "invalid virtual disk specifier");
> > > +    g_free(str);
> >
> > :(, g_free is called twice.
> 
> Oops. Good catch.

Oh, I thought you'd found a double free...

> 
> >
> > maybe we could have:
> >     vdev->valid=true;
> >     out:
> >       if (!vdev->valid)
> >         error_setg(...);
> >       g_free;

No, that's quite convoluted. I prefer separate 'fail' and 'success' paths, even 
if they both need to call g_free().

  Paul

> >
> > > diff --git a/include/hw/xen/xen-qdisk.h b/include/hw/xen/xen-qdisk.h
> > > new file mode 100644
> > > index 0000000000..ade0866037
> > > --- /dev/null
> > > +++ b/include/hw/xen/xen-qdisk.h
> > > @@ -0,0 +1,38 @@
> > > +/*
> > > + * Copyright (c) Citrix Systems Inc.
> > > + * All rights reserved.
> > > + */
> > > +
> > > +#ifndef HW_XEN_QDISK_H
> > > +#define HW_XEN_QDISK_H
> > > +
> > > +#include "hw/xen/xen-bus.h"
> > > +
> > > +typedef enum XenQdiskVdevType {
> > > +    XEN_QDISK_VDEV_TYPE_DP,
> >
> > Maybe we could set type_dp value to 1, so that, when vdev->type isn't
> > set, we can detect it later.
> 
> Rather than having the 'valid' bool? Yes, that would work.
> 
>   Paul
> 
> >
> >
> > > +    XEN_QDISK_VDEV_TYPE_XVD,
> > > +    XEN_QDISK_VDEV_TYPE_HD,
> > > +    XEN_QDISK_VDEV_TYPE_SD,
> > > +    XEN_QDISK_VDEV_TYPE__MAX
> > > +} XenQdiskVdevType;
> >
> > Thanks,
> >
> > --
> > Anthony PERARD


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel