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

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Anthony PERARD [mailto:anthony.perard@citrix.com]
> Sent: 04 December 2018 15:49
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: qemu-block@nongnu.org; qemu-devel@nongnu.org; xen-
> devel@lists.xenproject.org; Kevin Wolf <kwolf@redhat.com>; Max Reitz
> <mreitz@redhat.com>; Stefano Stabellini <sstabellini@kernel.org>
> Subject: Re: [PATCH 03/18] xen: introduce 'xen-qdisk'
> 
> On Tue, Dec 04, 2018 at 03:20:04PM +0000, Paul Durrant wrote:
> > > > +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.
> 
> There is libxl__devid_to_vdev() actually just after ;-) which calls
> encode_disk_name which is recursing.

Ah, I'll look for that. Currently having trouble reconciling the 'tq' -> 536 mapping in the doc.

  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.
> 
> Well, the 'valid' bool doesn't seems to always be check so it would be
> better. e.g. xen_qdisk_get_vdev() doesn't check `valid` before
> genereting a string. Then xen_qdisk_set_vdev could set `type` to invalid
> when it is invalid.
> 
> --
> Anthony PERARD

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