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

[Anthony PERARD]

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.

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