On Tue, Feb 14, 2017 at 07:02:32PM -0800, ashish mittal wrote:
> On Tue, Feb 14, 2017 at 2:34 PM, ashish mittal <ashmit...@gmail.com> wrote:
> > On Tue, Feb 14, 2017 at 12:51 PM, Jeff Cody <jc...@redhat.com> 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 <jc...@redhat.com> wrote:
> >>> > On Wed, Feb 08, 2017 at 09:23:33PM -0800, Ashish Mittal wrote:
> >>> >> From: Ashish Mittal <ashish.mit...@veritas.com>
> >>> >>
> >>> >> 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
