From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:55405) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UhErw-0003Qp-LR for qemu-devel@nongnu.org; Tue, 28 May 2013 04:03:05 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UhErp-0003Kg-K8 for qemu-devel@nongnu.org; Tue, 28 May 2013 04:02:56 -0400 Received: from mx1.redhat.com ([209.132.183.28]:36697) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UhErp-0003JW-Ah for qemu-devel@nongnu.org; Tue, 28 May 2013 04:02:49 -0400 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r4S82mDn024935 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 28 May 2013 04:02:48 -0400 Date: Tue, 28 May 2013 10:02:46 +0200 From: Stefan Hajnoczi Message-ID: <20130528080246.GD13368@stefanha-thinkpad.redhat.com> References: <1369723466-3288-1-git-send-email-famz@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1369723466-3288-1-git-send-email-famz@redhat.com> Subject: Re: [Qemu-devel] [PATCH] block: add read only to whitelist List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng Cc: kwolf@redhat.com, jcody@redhat.com, qemu-devel@nongnu.org On Tue, May 28, 2013 at 02:44:26PM +0800, Fam Zheng wrote: > We may want to include a driver in the whitelist for read only tasks > such as diagnosing or exporting guest data (with libguestfs as a good > example). This patch introduces the magic prefix ^ to include a driver > to the whitelist, but only enables qemu to open specific image > format readonly, and returns -ENOTSUP for RW opening. > > Example: To include vmdk readonly, and others read+write: > ./configure --block-drv-whitelist=qcow2,raw,file,qed,^vmdk This is great, thanks for tackling this. block/vmdk.c isn't suitable for running real VMs (read-write) since it's not optimized for concurrent I/O but it is usable for libguestfs (read-only). > Signed-off-by: Fam Zheng > --- > block.c | 43 +++++++++++++++++++++++++++---------------- > blockdev.c | 4 ++-- > configure | 2 ++ > include/block/block.h | 3 ++- > scripts/create_config | 9 ++++++++- > 5 files changed, 41 insertions(+), 20 deletions(-) > > diff --git a/block.c b/block.c > index 3f87489..e6a7270 100644 > --- a/block.c > +++ b/block.c > @@ -328,28 +328,40 @@ BlockDriver *bdrv_find_format(const char *format_name) > return NULL; > } > > -static int bdrv_is_whitelisted(BlockDriver *drv) > +static int bdrv_is_whitelisted(BlockDriver *drv, bool read_only) > { > - static const char *whitelist[] = { > - CONFIG_BDRV_WHITELIST > + static const char *whitelist_rw[] = { > + CONFIG_BDRV_WHITELIST_RW > + }; > + static const char *whitelist_ro[] = { > + CONFIG_BDRV_WHITELIST_RO > }; > const char **p; Please also make the ./configure lists separate. The funky ^ syntax is not obvious. Better to have: ./configure --block-drv-whitelist-rw=qcow2,raw,file,qed \ --block-drv-whitelist-ro=vmdk,vpc > > - if (!whitelist[0]) > + if (!whitelist_rw[0] && !whitelist_ro[0]) { > return 1; /* no whitelist, anything goes */ > + } > > - for (p = whitelist; *p; p++) { > + for (p = whitelist_rw; *p; p++) { > if (!strcmp(drv->format_name, *p)) { > return 1; > } > } > + if (read_only) { > + for (p = whitelist_ro; *p; p++) { > + if (!strcmp(drv->format_name, *p)) { > + return 1; > + } > + } > + } > return 0; > } > > -BlockDriver *bdrv_find_whitelisted_format(const char *format_name) > +BlockDriver *bdrv_find_whitelisted_format(const char *format_name, > + bool read_only) > { > BlockDriver *drv = bdrv_find_format(format_name); > - return drv && bdrv_is_whitelisted(drv) ? drv : NULL; > + return drv && bdrv_is_whitelisted(drv, read_only) ? drv : NULL; > } > > typedef struct CreateCo { > @@ -684,10 +696,6 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file, > > trace_bdrv_open_common(bs, filename ?: "", flags, drv->format_name); > > - if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv)) { > - return -ENOTSUP; > - } > - > /* bdrv_open() with directly using a protocol as drv. This layer is already > * opened, so assign it to bs (while file becomes a closed BlockDriverState) > * and return immediately. */ if (file != NULL && drv->bdrv_file_open) { bdrv_swap(file, bs); return 0; } I think your change is okay. You moved the check after this early return, but file is already opened so we passed the whitelist check already. This is a little tricky but it seems fine. Stefan