Re: [Qemu-block] [PATCH v2 for-2.7 6/8] block: Make bdrv_open() return a BDS

[Kevin Wolf]

Am 06.04.2016 um 19:57 hat Max Reitz geschrieben:
> There are no callers to bdrv_open() or bdrv_open_inherit() left that
> pass a pointer to a non-NULL BDS pointer as the first argument of these
> functions, so we can finally drop that parameter and just make them
> return the new BDS.
> 
> Generally, the following pattern is applied:
> 
> bs = NULL;
> ret = bdrv_open(, ..., _err);
> if (ret < 0) {
> error_propagate(errp, local_err);
> ...
> }
> 
> by
> 
> bs = bdrv_open(..., errp);
> if (!bs) {
> ret = -EINVAL;
> ...
> }
> 
> Of course, there are only a few instances where the pattern is really
> pure.
> 
> Signed-off-by: Max Reitz 

> @@ -1527,32 +1524,21 @@ static int bdrv_open_inherit(BlockDriverState **pbs, 
> const char *filename,
>  bool options_non_empty = options ? qdict_size(options) : false;
>  QDECREF(options);
>  
> -if (*pbs) {
> -error_setg(errp, "Cannot reuse an existing BDS when referencing "
> -   "another block device");
> -return -EINVAL;
> -}
> -
>  if (filename || options_non_empty) {
>  error_setg(errp, "Cannot reference an existing block device with 
> "
> "additional options or a new filename");
> -return -EINVAL;
> +return NULL;
>  }
>  
>  bs = bdrv_lookup_bs(reference, reference, errp);
>  if (!bs) {
> -return -ENODEV;
> +return NULL;
>  }
>  bdrv_ref(bs);
> -*pbs = bs;
> -return 0;
> +return bs;
>  }
>  
> -if (*pbs) {
> -bs = *pbs;
> -} else {
> -bs = bdrv_new();
> -}
> +bs = bdrv_new();
>  
>  /* NULL means an empty set of options */
>  if (options == NULL) {

While the following hunks remove the other instances, there's one
ret = -EINVAL left between here and the next hunk:

/* json: syntax counts as explicit options, as if in the QDict */
parse_json_protocol(options, , _err);
if (local_err) {
ret = -EINVAL;
goto fail;
}

> @@ -1589,7 +1575,6 @@ static int bdrv_open_inherit(BlockDriverState **pbs, 
> const char *filename,
>  drv = bdrv_find_format(drvname);
>  if (!drv) {
>  error_setg(errp, "Unknown driver: '%s'", drvname);
> -ret = -EINVAL;
>  goto fail;
>  }
>  }

With that fixed:
Reviewed-by: Kevin Wolf 

[Qemu-block] [PATCH v2 for-2.7 6/8] block: Make bdrv_open() return a BDS

[Max Reitz]

There are no callers to bdrv_open() or bdrv_open_inherit() left that
pass a pointer to a non-NULL BDS pointer as the first argument of these
functions, so we can finally drop that parameter and just make them
return the new BDS.

Generally, the following pattern is applied:

bs = NULL;
ret = bdrv_open(, ..., _err);
if (ret < 0) {
error_propagate(errp, local_err);
...
}

by

bs = bdrv_open(..., errp);
if (!bs) {
ret = -EINVAL;
...
}

Of course, there are only a few instances where the pattern is really
pure.

Signed-off-by: Max Reitz 
---
 block.c   | 123 +-
 block/block-backend.c |   6 +--
 block/vvfat.c |   8 ++--
 blockdev.c|  38 ++--
 include/block/block.h |   4 +-
 5 files changed, 63 insertions(+), 116 deletions(-)

diff --git a/block.c b/block.c
index b6d1796..355c4be 100644
--- a/block.c
+++ b/block.c
@@ -64,10 +64,12 @@ static QTAILQ_HEAD(, BlockDriverState) all_bdrv_states =
 static QLIST_HEAD(, BlockDriver) bdrv_drivers =
 QLIST_HEAD_INITIALIZER(bdrv_drivers);
 
-static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
- const char *reference, QDict *options, int flags,
- BlockDriverState *parent,
- const BdrvChildRole *child_role, Error **errp);
+static BlockDriverState *bdrv_open_inherit(const char *filename,
+   const char *reference,
+   QDict *options, int flags,
+   BlockDriverState *parent,
+   const BdrvChildRole *child_role,
+   Error **errp);
 
 /* If non-zero, use only whitelisted block drivers */
 static int use_bdrv_whitelist;
@@ -1338,14 +1340,13 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict 
*parent_options,
 qdict_put(options, "driver", qstring_from_str(bs->backing_format));
 }
 
-backing_hd = NULL;
-ret = bdrv_open_inherit(_hd,
-*backing_filename ? backing_filename : NULL,
-reference, options, 0, bs, _backing,
-errp);
-if (ret < 0) {
+backing_hd = bdrv_open_inherit(*backing_filename ? backing_filename : NULL,
+   reference, options, 0, bs, _backing,
+   errp);
+if (!backing_hd) {
 bs->open_flags |= BDRV_O_NO_BACKING;
 error_prepend(errp, "Could not open backing file: ");
+ret = -EINVAL;
 goto free_exit;
 }
 
@@ -1385,7 +1386,6 @@ BdrvChild *bdrv_open_child(const char *filename,
 BdrvChild *c = NULL;
 BlockDriverState *bs;
 QDict *image_options;
-int ret;
 char *bdref_key_dot;
 const char *reference;
 
@@ -1405,10 +1405,9 @@ BdrvChild *bdrv_open_child(const char *filename,
 goto done;
 }
 
-bs = NULL;
-ret = bdrv_open_inherit(, filename, reference, image_options, 0,
-parent, child_role, errp);
-if (ret < 0) {
+bs = bdrv_open_inherit(filename, reference, image_options, 0,
+   parent, child_role, errp);
+if (!bs) {
 goto done;
 }
 
@@ -1429,7 +1428,6 @@ static BlockDriverState 
*bdrv_append_temp_snapshot(BlockDriverState *bs,
 int64_t total_size;
 QemuOpts *opts = NULL;
 BlockDriverState *bs_snapshot;
-Error *local_err = NULL;
 int ret;
 
 /* if snapshot, we create a temporary backing file and open it
@@ -1468,12 +1466,10 @@ static BlockDriverState 
*bdrv_append_temp_snapshot(BlockDriverState *bs,
 qdict_put(snapshot_options, "driver",
   qstring_from_str("qcow2"));
 
-bs_snapshot = NULL;
-ret = bdrv_open(_snapshot, NULL, NULL, snapshot_options,
-flags, _err);
+bs_snapshot = bdrv_open(NULL, NULL, snapshot_options, flags, errp);
 snapshot_options = NULL;
-if (ret < 0) {
-error_propagate(errp, local_err);
+if (!bs_snapshot) {
+ret = -EINVAL;
 goto out;
 }
 
@@ -1504,10 +1500,12 @@ out:
  * should be opened. If specified, neither options nor a filename may be given,
  * nor can an existing BDS be reused (that is, *pbs has to be NULL).
  */
-static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
- const char *reference, QDict *options, int flags,
- BlockDriverState *parent,
- const BdrvChildRole *child_role, Error **errp)
+static BlockDriverState *bdrv_open_inherit(const char *filename,
+   const char *reference,
+   QDict *options, int flags,
+