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

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

[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 <famz@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 eb74510..87bf54c 100755
--- a/configure
+++ b/configure
@@ -1106,6 +1106,8 @@ echo "  --enable-cocoa           enable Cocoa (default on Mac OS X)"
 echo "  --audio-drv-list=LIST    set audio drivers list:"
 echo "                           Available drivers: $audio_possible_drivers"
 echo "  --block-drv-whitelist=L  set block driver whitelist"
+echo "                           prefix with ^ to include driver readonly"
+echo "                           e.g. --block-drv-whitelist=raw,file,^vmdk"
 echo "                           (affects only QEMU, not qemu-img)"
 echo "  --enable-mixemu          enable mixer emulation"
 echo "  --disable-xen            disable xen backend driver support"
diff --git a/include/block/block.h b/include/block/block.h
index 1251c5c..5604418 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -124,7 +124,8 @@ void bdrv_init(void);
 void bdrv_init_with_whitelist(void);
 BlockDriver *bdrv_find_protocol(const char *filename);
 BlockDriver *bdrv_find_format(const char *format_name);
-BlockDriver *bdrv_find_whitelisted_format(const char *format_name);
+BlockDriver *bdrv_find_whitelisted_format(const char *format_name,
+                                          bool readonly);
 int bdrv_create(BlockDriver *drv, const char* filename,
     QEMUOptionParameter *options);
 int bdrv_create_file(const char* filename, QEMUOptionParameter *options);
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;
       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%=*}
-- 
1.8.2.3

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

[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 <famz@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

[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 <famz@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

[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

[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