From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:57918)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <kwolf@redhat.com>) id 1ao9DQ-0001DI-EH
	for qemu-devel@nongnu.org; Thu, 07 Apr 2016 08:39:20 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <kwolf@redhat.com>) id 1ao9DK-0006HB-QB
	for qemu-devel@nongnu.org; Thu, 07 Apr 2016 08:39:16 -0400
Date: Thu, 7 Apr 2016 14:39:03 +0200
From: Kevin Wolf <kwolf@redhat.com>
Message-ID: <20160407123903.GG4509@noname.redhat.com>
References: <1459965434-21531-1-git-send-email-mreitz@redhat.com>
	<1459965434-21531-7-git-send-email-mreitz@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1459965434-21531-7-git-send-email-mreitz@redhat.com>
Subject: Re: [Qemu-devel] [PATCH v2 for-2.7 6/8] block: Make bdrv_open()
	return a BDS
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Max Reitz <mreitz@redhat.com>
Cc: Alberto Garcia <berto@igalia.com>, qemu-devel@nongnu.org, qemu-block@nongnu.org

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 <mreitz@redhat.com>

> @@ -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 <kwolf@redhat.com>