Re: [PATCH 1/4] rbd: have rbd_dev_image_id() set format 1 image id

[Josh Durgin <josh.durgin@inktank.com>]

Reviewed-by: Josh Durgin <josh.durgin@inktank.com>

On 04/26/2013 07:52 AM, Alex Elder wrote:
> Currently, rbd_dev_probe() assumes that any error returned by
> rbd_dev_image_id() is most likely -ENOENT, and responds by
> calling the format 1 probe routine, rbd_dev_v1_probe().  Then,
> at the top of rbd_dev_v1_probe(), an empty string is allocated
> for the image id.
>
> This is sort of unbalanced.  Fix this by having rbd_dev_image_id()
> look for -ENOENT from its "get_id" method call.  If that is seen,
> have it allocate the empty string there rather than depending on
> rbd_dev_v1_probe() to do it.
>
> Also drop a redundant hunk of code in rbd_dev_image_id().
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   47 ++++++++++++++++++++++-------------------------
>   1 file changed, 22 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 9e38967..0774ae1 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -4476,12 +4476,7 @@ static int rbd_dev_image_id(struct rbd_device
> *rbd_dev)
>   	size_t size;
>   	char *object_name;
>   	void *response;
> -	void *p;
> -
> -	/* If we already have it we don't need to look it up */
> -
> -	if (rbd_dev->spec->image_id)
> -		return 0;
> +	char *image_id;
>
>   	/*
>   	 * When probing a parent image, the image id is already
> @@ -4511,24 +4506,28 @@ static int rbd_dev_image_id(struct rbd_device
> *rbd_dev)
>   		goto out;
>   	}
>
> +	/* If it doesn't exist we'll assume it's a format 1 image */
> +
>   	ret = rbd_obj_method_sync(rbd_dev, object_name,
>   				"rbd", "get_id", NULL, 0,
>   				response, RBD_IMAGE_ID_LEN_MAX, NULL);
>   	dout("%s: rbd_obj_method_sync returned %d\n", __func__, ret);
> -	if (ret < 0)
> -		goto out;
> +	if (ret == -ENOENT) {
> +		image_id = kstrdup("", GFP_KERNEL);
> +		ret = image_id ? 0 : -ENOMEM;
> +	} else if (ret > sizeof (__le32)) {
> +		void *p = response;
>
> -	p = response;
> -	rbd_dev->spec->image_id = ceph_extract_encoded_string(&p,
> -						p + ret,
> +		image_id = ceph_extract_encoded_string(&p, p + ret,
>   						NULL, GFP_NOIO);
> -	ret = 0;
> -
> -	if (IS_ERR(rbd_dev->spec->image_id)) {
> -		ret = PTR_ERR(rbd_dev->spec->image_id);
> -		rbd_dev->spec->image_id = NULL;
> +		ret = IS_ERR(image_id) ? PTR_ERR(image_id) : 0;
>   	} else {
> -		dout("image_id is %s\n", rbd_dev->spec->image_id);
> +		ret = -EINVAL;
> +	}
> +
> +	if (!ret) {
> +		rbd_dev->spec->image_id = image_id;
> +		dout("image_id is %s\n", image_id);
>   	}
>   out:
>   	kfree(response);
> @@ -4542,12 +4541,6 @@ static int rbd_dev_v1_probe(struct rbd_device
> *rbd_dev)
>   	int ret;
>   	size_t size;
>
> -	/* Version 1 images have no id; empty string is used */
> -
> -	rbd_dev->spec->image_id = kstrdup("", GFP_KERNEL);
> -	if (!rbd_dev->spec->image_id)
> -		return -ENOMEM;
> -
>   	/* Record the header object name for this rbd image. */
>
>   	size = strlen(rbd_dev->spec->image_name) + sizeof (RBD_SUFFIX);
> @@ -4794,9 +4787,13 @@ static int rbd_dev_probe(struct rbd_device *rbd_dev)
>   	 */
>   	ret = rbd_dev_image_id(rbd_dev);
>   	if (ret)
> -		ret = rbd_dev_v1_probe(rbd_dev);
> -	else
> +		return ret;
> +
> +	rbd_assert(rbd_dev->spec->image_id);
> +	if (*rbd_dev->spec->image_id)
>   		ret = rbd_dev_v2_probe(rbd_dev);
> +	else
> +		ret = rbd_dev_v1_probe(rbd_dev);
>   	if (ret) {
>   		dout("probe failed, returning %d\n", ret);
>