From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37479) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cdk4y-0007Jf-7s for qemu-devel@nongnu.org; Tue, 14 Feb 2017 15:52:05 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1cdk4v-0002Lh-33 for qemu-devel@nongnu.org; Tue, 14 Feb 2017 15:52:04 -0500 Received: from mx1.redhat.com ([209.132.183.28]:38724) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1cdk4u-0002LI-QD for qemu-devel@nongnu.org; Tue, 14 Feb 2017 15:52:01 -0500 Date: Tue, 14 Feb 2017 15:51:58 -0500 From: Jeff Cody Message-ID: <20170214205158.GT27752@localhost.localdomain> References: <1486617814-5420-1-git-send-email-Ashish.Mittal@veritas.com> <20170209062928.GI27752@localhost.localdomain> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH v8 1/2] block/vxhs.c: Add support for a new block device type called "vxhs" List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: ashish mittal Cc: qemu-devel , Paolo Bonzini , Kevin Wolf , Markus Armbruster , "Daniel P. Berrange" , Fam Zheng , Ashish Mittal , Stefan Hajnoczi , John Ferlan , Buddhi Madhav , Suraj Singh , Nitin Jerath , Peter Maydell , Ketan Nilangekar , Abhijit Dey On Thu, Feb 09, 2017 at 01:24:58AM -0800, ashish mittal wrote: > On Wed, Feb 8, 2017 at 10:29 PM, Jeff Cody wrote: > > On Wed, Feb 08, 2017 at 09:23:33PM -0800, Ashish Mittal wrote: > >> From: Ashish Mittal > >> > >> 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. [...] > >> + > >> +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? > > > > > > 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); > >> +} > >> +