From patchwork Wed Feb 15 03:02:32 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ashish Mittal X-Patchwork-Id: 9573275 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 1F2186045F for ; Wed, 15 Feb 2017 03:03:10 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 17A1D283E1 for ; Wed, 15 Feb 2017 03:03:10 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 0B9A228427; Wed, 15 Feb 2017 03:03:10 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=2.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, FREEMAIL_FROM, RCVD_IN_DNSWL_HI, T_DKIM_INVALID autolearn=ham version=3.3.1 Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 49F2F283E1 for ; Wed, 15 Feb 2017 03:03:09 +0000 (UTC) Received: from localhost ([::1]:38206 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cdps4-0000UQ-8o for patchwork-qemu-devel@patchwork.kernel.org; Tue, 14 Feb 2017 22:03:08 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39732) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cdpra-0000Rw-96 for qemu-devel@nongnu.org; Tue, 14 Feb 2017 22:02:40 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1cdprY-0004uf-8w for qemu-devel@nongnu.org; Tue, 14 Feb 2017 22:02:38 -0500 Received: from mail-it0-x241.google.com ([2607:f8b0:4001:c0b::241]:34866) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1cdprY-0004uT-2g for qemu-devel@nongnu.org; Tue, 14 Feb 2017 22:02:36 -0500 Received: by mail-it0-x241.google.com with SMTP id 203so8597271ith.2 for ; Tue, 14 Feb 2017 19:02:35 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=R2LzP28gkyPPp9T05Yzi1/KAdSMwM3EtT/yRi8o12tk=; b=FRRKBpcZ3ZSSM3ZNYkzE7im8AdwkzkwfhlIJmUTfGVnYVkB3cFKjUZ/8kt2/IsoN1d cmmYl+63xR6OthQMoeJuAHZ7Dg68aJqjItkphAyb/wNcMiQ/IrLRJe6Siv4y9sg+Mr5F oyMYH4cpS9nBp2ZJsiv2+nueDmMwIDJj7d+2XpzqbkDyNEBmSTeLuL3dH8JeNcNX0PW/ HjRuZQCtCSrRz+hQ11hLLoExBb1WaGrk8ww/P9SVcRYjFJhtFjIhdP6Oq10kFCUfI9NJ chgGUSCuof76TtmJ8y9KDgLQtEoflOxiDlHaA0jj+1w6v+6tOlH0LRrMtzLqFaHyD6Z2 poSA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=R2LzP28gkyPPp9T05Yzi1/KAdSMwM3EtT/yRi8o12tk=; b=P+cITSB+PY20DFlnc/Ge4mjyazXB0RgsX9O8t9g5NTwdTr1H6TWGLmd5m6HU2y7I3v 4A0H+iAe8z3rAuDHl8nbPpmGGWl0IXSYQpJLdWUJ6WfkWM6naw7FN9xPlme7Ca0ipJhl Bc3zqv7zHzs8JzasbfD1AE+gRCs+1CzuNAkdfCFwy76+6Pjbr5818ED0fYqEPuiGrT1C nujE83uz0D0ByeCBMAxbZ2K2mejQGgMo8th5oX2V7RG2CyQ4nT9X9lxtJsQIyvUTBxBy DfBYk8QgfVqshYVP0xyt5bZOLRAZXCy0ds+1OADU51f7evlkxRTDrFJX166rbUxaU0fJ WNSA== X-Gm-Message-State: AMke39kQmXLZV3AevqssGywLER7nplQKe8Fzei28C4JfJrmyupN04Y0+WQqSaeEz4sRp85boqMsCwf/xTl115g== X-Received: by 10.36.93.213 with SMTP id w204mr7345698ita.60.1487127753898; Tue, 14 Feb 2017 19:02:33 -0800 (PST) MIME-Version: 1.0 Received: by 10.79.137.4 with HTTP; Tue, 14 Feb 2017 19:02:32 -0800 (PST) In-Reply-To: References: <1486617814-5420-1-git-send-email-Ashish.Mittal@veritas.com> <20170209062928.GI27752@localhost.localdomain> <20170214205158.GT27752@localhost.localdomain> From: ashish mittal Date: Tue, 14 Feb 2017 19:02:32 -0800 Message-ID: To: Jeff Cody X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 2607:f8b0:4001:c0b::241 Subject: Re: [Qemu-devel] [PATCH v8 1/2] block/vxhs.c: Add support for a new block device type called "vxhs" X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Kevin Wolf , Peter Maydell , "Venkatesha M.G." , Ashish Mittal , Fam Zheng , Nitin Jerath , Markus Armbruster , Rakesh Ranjan , qemu-devel , Suraj Singh , Ketan Nilangekar , Abhijit Dey , Stefan Hajnoczi , Paolo Bonzini , Buddhi Madhav , John Ferlan Errors-To: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: "Qemu-devel" X-Virus-Scanned: ClamAV using ClamSMTP On Tue, Feb 14, 2017 at 2:34 PM, ashish mittal wrote: > On Tue, Feb 14, 2017 at 12:51 PM, Jeff Cody 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 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. >> > > (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. Does the following diff look ok for the refcnt and iio_fini() change? Thanks, Ashish > Will consider this in the next patch. > > Regards, > Ashish 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); }