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

[ashish mittal]

On Tue, Feb 14, 2017 at 2:34 PM, ashish mittal <address@hidden> wrote:
> On Tue, Feb 14, 2017 at 12:51 PM, Jeff Cody <address@hidden> wrote:
>> 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.
>>
>
> (1) The default behavior would be to disallow access to any vdisks by
> the non qemu-kvm binaries. qemu-kvm would use the actual instance ID
> for authentication.
> (2) Depending on the workflow, HyperScale controller can choose to
> grant *temporary* access to specific vdisks by qemu-img, qemu-io
> binaries (identified by the default VXHS_UUID_DEF above).
> (3) This information, described in #2, would be communicated by the
> HyperScale controller to the actual proprietary VxHS server (running
> on each compute) that does the authentication/SSL.
> (4) The HyperScale controller, in this way, can grant/revoke access
> for specific vdisks not just to clients with VXHS_UUID_DEF instance
> ID, but also the actual VM instances.
>
>> [...]
>>
>>> >> +
>>> >> +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?
>>> >
>
> (1) qemu_uuid_is_null(&qemu_uuid) is true for qemu-io, qemu-img that
> do not define a instance ID. We end up using the default VXHS_UUID_DEF
> ID for them, and authenticating them as described above.
>
> (2) For the other case 'else', we convert the uuid to a char * using
> qemu_uuid_unparse(), and pass the resulting char * uuid in variable
> 'out' to libvxhs.
>
>>> >
>>> > 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);
>>> >> +}
>>> >> +
>

Per my understanding, device open and close are serialized, therefore
I would not need to use the refcnt under a lock.
Does the following diff look ok for the refcnt and iio_fini() change?

diff --git a/block/vxhs.c b/block/vxhs.c
index f1b5f1c..d07a461 100644
--- a/block/vxhs.c
+++ b/block/vxhs.c
@@ -27,7 +27,7 @@

 QemuUUID qemu_uuid __attribute__ ((weak));

-static int lib_init_failed;
+static uint32_t refcnt;

 typedef enum {
     VDISK_AIO_READ,
@@ -232,9 +232,24 @@ static int vxhs_open(BlockDriverState *bs, QDict *options,
     char *str = NULL;
     int ret = 0;

-    if (lib_init_failed) {
-        return -ENODEV;
+    if (refcnt == 0) {
+        char out[UUID_FMT_LEN + 1];
+        if (qemu_uuid_is_null(&qemu_uuid)) {
+            if (iio_init(QNIO_VERSION, vxhs_iio_callback, VXHS_UUID_DEF))
+                return -ENODEV;
+        } else {
+            qemu_uuid_unparse(&qemu_uuid, out);
+            if (iio_init(QNIO_VERSION, vxhs_iio_callback, out))
+                return -ENODEV;
+        }
     }
+
+    /*
+     * Increment refcnt before actual open.
+     * We will decrement it if there is an error.
+     */
+    refcnt++;
+
     opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
     qemu_opts_absorb_qdict(opts, options, &local_err);
     if (local_err) {
@@ -323,6 +338,13 @@ out:
     qemu_opts_del(opts);

     if (ret < 0) {
+        if (refcnt == 1) {
+            /*
+             * First device open resulted in error.
+             */
+            iio_fini();
+        }
+        refcnt--;
         error_propagate(errp, local_err);
         g_free(s->vdisk_hostinfo.host);
         g_free(s->vdisk_guid);
@@ -428,6 +450,10 @@ static void vxhs_close(BlockDriverState *bs)
         s->vdisk_hostinfo.dev_handle = NULL;
     }

+    if (--refcnt == 0) {
+        iio_fini();
+    }
+
     /*
      * Free the dynamically allocated host string
      */
@@ -484,15 +510,6 @@ static BlockDriver bdrv_vxhs = {

 static void bdrv_vxhs_init(void)
 {
-    char out[37];
-
-    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);
-    }
-
     bdrv_register(&bdrv_vxhs);
 }

Thanks,
Ashish

> Will consider this in the next patch.
>
> Regards,
> Ashish