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);
>> >> +}
>> >> +
Will consider this in the next patch.
Regards,
Ashish
