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

[Stefan Hajnoczi <stefanha@redhat.com>]

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