Re: [Qemu-devel] [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(&bs, ..., &local_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 <address@hidden>

> @@ -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, &filename, &local_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 <address@hidden>