| Message ID | CAAo6VWPUL7=jotUzgbA7ko-9+WjeqL7kxWF_1LVLQxs8Y6ajbg@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Feb 14, 2017 at 07:02:32PM -0800, ashish mittal wrote: > On Tue, Feb 14, 2017 at 2:34 PM, ashish mittal <ashmit602@gmail.com> wrote: > > On Tue, Feb 14, 2017 at 12:51 PM, Jeff Cody <jcody@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 <jcody@redhat.com> wrote: > >>> > On Wed, Feb 08, 2017 at 09:23:33PM -0800, Ashish Mittal wrote: > >>> >> From: Ashish Mittal <ashish.mittal@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
Thanks! Will change accordingly in the next patch. On Tue, Feb 14, 2017 at 7:54 PM, Jeff Cody <jcody@redhat.com> wrote: > On Tue, Feb 14, 2017 at 07:02:32PM -0800, ashish mittal wrote: >> On Tue, Feb 14, 2017 at 2:34 PM, ashish mittal <ashmit602@gmail.com> wrote: >> > On Tue, Feb 14, 2017 at 12:51 PM, Jeff Cody <jcody@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 <jcody@redhat.com> wrote: >> >>> > On Wed, Feb 08, 2017 at 09:23:33PM -0800, Ashish Mittal wrote: >> >>> >> From: Ashish Mittal <ashish.mittal@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
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); }