Re: [Qemu-devel] [PATCH v8 1/2] block/vxhs.c: Add support for a new block device type called "vxhs"

[Jeff Cody]

On Thu, Feb 09, 2017 at 01:24:58AM -0800, ashish mittal wrote:
> On Wed, Feb 8, 2017 at 10:29 PM, Jeff Cody <address@hidden> wrote:
> > On Wed, Feb 08, 2017 at 09:23:33PM -0800, Ashish Mittal wrote:
> >> From: Ashish Mittal <address@hidden>
> >>
> >> Source code for the qnio library that this code loads can be downloaded 
> >> from:
> >> https://github.com/VeritasHyperScale/libqnio.git
> >>
> >> Sample command line using JSON syntax:
> >> ./x86_64-softmmu/qemu-system-x86_64 -name instance-00000008 -S -vnc 
> >> 0.0.0.0:0
> >> -k en-us -vga cirrus -device 
> >> virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x5
> >> -msg timestamp=on
> >> 'json:{"driver":"vxhs","vdisk-id":"c3e9095a-a5ee-4dce-afeb-2a59fb387410",
> >> "server":{"host":"172.172.17.4","port":"9999"}}'
> >>
> >> Sample command line using URI syntax:
> >> qemu-img convert -f raw -O raw -n
> >> /var/lib/nova/instances/_base/0c5eacd5ebea5ed914b6a3e7b18f1ce734c386ad
> >> vxhs://192.168.0.1:9999/c6718f6b-0401-441d-a8c3-1f0064d75ee0
> >>

[...]

> >> +#define VXHS_OPT_FILENAME           "filename"
> >> +#define VXHS_OPT_VDISK_ID           "vdisk-id"
> >> +#define VXHS_OPT_SERVER             "server"
> >> +#define VXHS_OPT_HOST               "host"
> >> +#define VXHS_OPT_PORT               "port"
> >> +#define VXHS_UUID_DEF "12345678-1234-1234-1234-123456789012"
> >
> > What is this?  It is not a valid UUID; is the value significant?
> >
> 
> This value gets passed to libvxhs for binaries like qemu-io, qemu-img
> that do not have an Instance ID. We can use this default ID to control
> access to specific vdisks by these binaries. qemu-kvm will pass the
> actual instance ID, and therefore will not use this default value.
> 
> Will reply to other queries soon.
>

If you are going to call it a UUID, it should adhere to the RFC 4122 spec.
You can easily generate a compliant UUID with uuidgen.  However:

Can you explain more about how you are using this to control access by
qemu-img and qemu-io?  Looking at libqnio, it looks like this is used to
determine at runtime which TLS certs to use based off of a
pathname/filename, which is then how I presume you are controlling access.
See Daniel's email regarding TLS certificates.

[...]

> >> +
> >> +static void bdrv_vxhs_init(void)
> >> +{
> >> +    char out[37];

Additional point: this should be sized as UUID_FMT_LEN + 1, not 37, but I
suspect this code is changing anyways.

> >> +
> >> +    if (qemu_uuid_is_null(&qemu_uuid)) {
> >> +        lib_init_failed = iio_init(QNIO_VERSION, vxhs_iio_callback, 
> >> VXHS_UUID_DEF);
> >> +    } else {
> >> +        qemu_uuid_unparse(&qemu_uuid, out);
> >> +        lib_init_failed = iio_init(QNIO_VERSION, vxhs_iio_callback, out);
> >> +    }
> >> +
> >
> > [1]
> >
> > Can you explain what is going on here with the qemu_uuid check?
> >
> >
> > You also can't do this here.  This init function is just to register the
> > driver (e.g. populate the BlockDriver list).  You shouldn't be doing
> > anything other than the bdrv_register() call here.
> >
> > Since you want to run this iio_init only once, I would recommend doing it in
> > the vxhs_open() call, and using a ref counter.  That way, you can also call
> > iio_fini() to finish releasing resources once the last device is closed.
> >
> > This was actually a suggestion I had before, which you then incorporated
> > into v6, but it appears all the refcnt code has gone away for v7/v8.
> >
> >
> >> +    bdrv_register(&bdrv_vxhs);
> >> +}
> >> +