[Qemu-devel] [PATCH] block: add read only to whitelist

2013-05-28 Thread Fam Zheng 
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

Signed-off-by: Fam Zheng f...@redhat.com
---
 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;
 
-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. */
@@ -698,9 +706,15 @@ static int bdrv_open_common(BlockDriverState *bs, 
BlockDriverState *file,
 
 bs-open_flags = flags;
 bs-buffer_alignment = 512;
+open_flags = bdrv_open_flags(bs, flags);
+bs-read_only = !(open_flags  BDRV_O_RDWR);
+
+if (use_bdrv_whitelist  !bdrv_is_whitelisted(drv, bs-read_only)) {
+return -ENOTSUP;
+}
 
 assert(bs-copy_on_read == 0); /* bdrv_new() and bdrv_close() make it so */
-if ((flags  BDRV_O_RDWR)  (flags  BDRV_O_COPY_ON_READ)) {
+if (!bs-read_only  (flags  BDRV_O_COPY_ON_READ)) {
 bdrv_enable_copy_on_read(bs);
 }
 
@@ -714,9 +728,6 @@ static int bdrv_open_common(BlockDriverState *bs, 
BlockDriverState *file,
 bs-opaque = g_malloc0(drv-instance_size);
 
 bs-enable_write_cache = !!(flags  BDRV_O_CACHE_WB);
-open_flags = bdrv_open_flags(bs, flags);
-
-bs-read_only = !(open_flags  BDRV_O_RDWR);
 
 /* Open the image, either directly or using a protocol */
 if (drv-bdrv_file_open) {
@@ -801,7 +812,7 @@ int bdrv_file_open(BlockDriverState **pbs, const char 
*filename,
 /* Find the right block driver */
 drvname = qdict_get_try_str(options, driver);
 if (drvname) {
-drv = bdrv_find_whitelisted_format(drvname);
+drv = bdrv_find_whitelisted_format(drvname, !(flags  BDRV_O_RDWR));
 qdict_del(options, driver);
 } else if (filename) {
 drv = bdrv_find_protocol(filename);
diff --git a/blockdev.c b/blockdev.c
index d1ec99a..b9b2d10 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -477,7 +477,7 @@ DriveInfo *drive_init(QemuOpts *all_opts, 
BlockInterfaceType block_default_type)
 error_printf(\n);
 return NULL;
 }
-drv = bdrv_find_whitelisted_format(buf);
+drv = bdrv_find_whitelisted_format(buf, ro);
 if (!drv) {
 error_report('%s' invalid format, buf);
 return NULL;
@@ -1096,7 +1096,7 @@ void qmp_change_blockdev(const char *device, const char 
*filename,
 }
 
 if (format) {
-drv = bdrv_find_whitelisted_format(format);
+drv = bdrv_find_whitelisted_format(format, bs-read_only);
 if (!drv) {
 error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
 return;
diff --git a/configure b/configure
index

Re: [Qemu-devel] [PATCH] block: add read only to whitelist

2013-05-28 Thread Kevin Wolf 
Am 28.05.2013 um 08:44 hat Fam Zheng geschrieben:
 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
 
 Signed-off-by: Fam Zheng f...@redhat.com

^ looks a bit like not, so this is potentially confusing syntax. Why
not simply add a second option?

  ./configure --block-drv-whitelist=qcow2,raw,file,qed \
--block-drv-ro-whitelist=vmdk

Kevin

Re: [Qemu-devel] [PATCH] block: add read only to whitelist

2013-05-28 Thread Stefan Hajnoczi 
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 f...@redhat.com
 ---
  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

Re: [Qemu-devel] [PATCH] block: add read only to whitelist

2013-05-28 Thread Paolo Bonzini 
Il 28/05/2013 08:44, Fam Zheng ha scritto:
 diff --git a/scripts/create_config b/scripts/create_config
 index c471e8c..2dfda3e 100755
 --- a/scripts/create_config
 +++ b/scripts/create_config
 @@ -35,11 +35,18 @@ case $line in
  echo 
  ;;
   CONFIG_BDRV_WHITELIST=*)
 -echo #define CONFIG_BDRV_WHITELIST \\
 +echo #define CONFIG_BDRV_WHITELIST_RW \\
  for drv in ${line#*=}; do
 +  [[ ${drv} = ^* ]]  continue;

I didn't know about this feature.  Can you point me to the documentation?

You would need to change the #! header to #! /bin/bash if you use it,
but since you have to respin anyway, I'd ask you to use case instead. :)

Paolo

echo \${drv}\,\\
  done
  echo NULL
 +echo #define CONFIG_BDRV_WHITELIST_RO \\
 +for drv in ${line#*=}; do
 +  [[ ${drv} != ^* ]]  continue;
 +  echo \${drv#^}\,\\
 +done
 +echo NULL
  ;;
   CONFIG_*=y) # configuration
  name=${line%=*}

Re: [Qemu-devel] [PATCH] block: add read only to whitelist

2013-05-28 Thread Fam Zheng 
On Tue, 05/28 10:10, Paolo Bonzini wrote:
 Il 28/05/2013 08:44, Fam Zheng ha scritto:
  diff --git a/scripts/create_config b/scripts/create_config
  index c471e8c..2dfda3e 100755
  --- a/scripts/create_config
  +++ b/scripts/create_config
  @@ -35,11 +35,18 @@ case $line in
   echo 
   ;;
CONFIG_BDRV_WHITELIST=*)
  -echo #define CONFIG_BDRV_WHITELIST \\
  +echo #define CONFIG_BDRV_WHITELIST_RW \\
   for drv in ${line#*=}; do
  +  [[ ${drv} = ^* ]]  continue;
 
 I didn't know about this feature.  Can you point me to the documentation?

Yes, it is bash only, I'd better use a more compatible way.

http://mywiki.wooledge.org/glob

 
 You would need to change the #! header to #! /bin/bash if you use it,
 but since you have to respin anyway, I'd ask you to use case instead. :)

As Stefan and Kevin pointed out, I'll replace ^ prefix with a separate
configure option, it'll become
   CONFIG_BDRV_WHITELIST_RW=*)
...
;;
   CONFIG_BDRV_WHITELIST_RO=*)
...
;;

Then I won't need globbing.

-- 
Fam