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

[Jeff Cody]

On Tue, Feb 14, 2017 at 07:02:32PM -0800, ashish mittal wrote:
> 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.

Correct.

> Does the following diff look ok for the refcnt and iio_fini() change?
>

Disclaimer, the following was compiled in my mind only.

Create a wrapper for the ref, and initialize automatically when appropriate.
For instance:



/* Only refs after successful init */
int vxhs_init_and_ref() {
    if (vxhs_ref == 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;
        }
    }
    vxhs_ref++;
    return 0;
}


And then another wrapper to unref it, and then call iio_fini() as
appropriate:


void vxhs_unref() {
    if (vxhs_ref && --vxhs_ref == 0) {
        iio_fini();
    }
}

Now whenever you ref or unref the usage counter, everything happens
correctly automatically.

Then the rest of the patch becomes:

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

Minor nit: I'd call it vxhs_ref (just a preference).

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

This block then just becomes:

      ret = vxhs_init_and_ref();
      if (ret < 0) {
          return ret;
      }


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

This hunk can just be replaced with:

    vxhs_unref();

>          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();
> +    }
> +

Same here:

    vxhs_unref();


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

-Jeff