[Qemu-devel] [PATCH] block: Improve driver whitelist checks

[Qemu-devel] [PATCH] block: Improve driver whitelist checks

[Kevin Wolf]

The main intent of this patch is to consolidate the whitelist checks to
a single point in the code instead of spreading it everywhere. This adds
a nicer error message for read-only whitelisting, too, in places where
it was still missing.

The patch also contains a bonus bug fix: By finding the format first in
bdrv_open() and then independently checking against the whitelist only
later, we avoid the case that use of a non-whitelisted format results in
probing rather than an error message. Previously, this could happen when
using the driver=... option.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c    | 10 +++++++---
 blockdev.c |  8 ++------
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/block.c b/block.c
index beea027..84c0eac 100644
--- a/block.c
+++ b/block.c
@@ -769,7 +769,11 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
     bs->read_only = !(open_flags & BDRV_O_RDWR);
 
     if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv, bs->read_only)) {
-        error_setg(errp, "Driver '%s' is not whitelisted", drv->format_name);
+        error_setg(errp,
+                   !bs->read_only && bdrv_is_whitelisted(drv, true)
+                        ? "Driver '%s' can only be used for read-only devices"
+                        : "Driver '%s' is not whitelisted",
+                   drv->format_name);
         return -ENOTSUP;
     }
 
@@ -881,7 +885,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, !(flags & BDRV_O_RDWR));
+        drv = bdrv_find_format(drvname);
         if (!drv) {
             error_setg(errp, "Unknown driver '%s'", drvname);
         }
@@ -1123,7 +1127,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
     /* Find the right image format driver */
     drvname = qdict_get_try_str(options, "driver");
     if (drvname) {
-        drv = bdrv_find_whitelisted_format(drvname, !(flags & BDRV_O_RDWR));
+        drv = bdrv_find_format(drvname);
         qdict_del(options, "driver");
     }
 
diff --git a/blockdev.c b/blockdev.c
index 92029d8..5f3cece 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -468,13 +468,9 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts,
             return NULL;
         }
 
-        drv = bdrv_find_whitelisted_format(buf, ro);
+        drv = bdrv_find_format(buf);
         if (!drv) {
-            if (!ro && bdrv_find_whitelisted_format(buf, !ro)) {
-                error_report("'%s' can be only used as read-only device.", buf);
-            } else {
-                error_report("'%s' invalid format", buf);
-            }
+            error_report("'%s' invalid format", buf);
             return NULL;
         }
     }
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH] block: Improve driver whitelist checks

[Fam Zheng]

On Thu, 10/10 11:57, Kevin Wolf wrote:
> The main intent of this patch is to consolidate the whitelist checks to
> a single point in the code instead of spreading it everywhere. This adds
> a nicer error message for read-only whitelisting, too, in places where
> it was still missing.
> 
> The patch also contains a bonus bug fix: By finding the format first in
> bdrv_open() and then independently checking against the whitelist only
> later, we avoid the case that use of a non-whitelisted format results in
> probing rather than an error message. Previously, this could happen when
> using the driver=... option.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c    | 10 +++++++---
>  blockdev.c |  8 ++------
>  2 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/block.c b/block.c
> index beea027..84c0eac 100644
> --- a/block.c
> +++ b/block.c
> @@ -769,7 +769,11 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
>      bs->read_only = !(open_flags & BDRV_O_RDWR);
>  
>      if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv, bs->read_only)) {
> -        error_setg(errp, "Driver '%s' is not whitelisted", drv->format_name);
> +        error_setg(errp,
> +                   !bs->read_only && bdrv_is_whitelisted(drv, true)
> +                        ? "Driver '%s' can only be used for read-only devices"
> +                        : "Driver '%s' is not whitelisted",
> +                   drv->format_name);
>          return -ENOTSUP;
>      }
>  
> @@ -881,7 +885,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, !(flags & BDRV_O_RDWR));
> +        drv = bdrv_find_format(drvname);
>          if (!drv) {
>              error_setg(errp, "Unknown driver '%s'", drvname);
>          }
> @@ -1123,7 +1127,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
>      /* Find the right image format driver */
>      drvname = qdict_get_try_str(options, "driver");
>      if (drvname) {
> -        drv = bdrv_find_whitelisted_format(drvname, !(flags & BDRV_O_RDWR));
> +        drv = bdrv_find_format(drvname);
>          qdict_del(options, "driver");
>      }
>  
> diff --git a/blockdev.c b/blockdev.c
> index 92029d8..5f3cece 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -468,13 +468,9 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts,
>              return NULL;
>          }
>  
> -        drv = bdrv_find_whitelisted_format(buf, ro);
> +        drv = bdrv_find_format(buf);
>          if (!drv) {
> -            if (!ro && bdrv_find_whitelisted_format(buf, !ro)) {
> -                error_report("'%s' can be only used as read-only device.", buf);
> -            } else {
> -                error_report("'%s' invalid format", buf);
> -            }
> +            error_report("'%s' invalid format", buf);
>              return NULL;
>          }
>      }

This is much cleaner now. Thanks.

Reviewed-by: Fam Zheng <famz@redhat.com>