Re: [PATCH 06/33] libceph: fixup error handling in osdmap_decode()

[Alex Elder <elder@ieee.org>]

On 03/27/2014 01:17 PM, Ilya Dryomov wrote:
> The existing error handling scheme requires resetting err to -EINVAL
> prior to calling any ceph_decode_* macro.  This is ugly and fragile,
> and there already are a few places where we would return 0 on error,
> due to a missing reset.  Fix this by adding a special e_inval label to
> be used by all ceph_decode_* macros.

I don't see where it's returning 0 on error, but I think this
is a good change anyway.

I'd use "einval" or "err_inval" instead of "e_inval".  But
no matter.

Looks good.

Reviewed-by: Alex Elder <elder@linaro.org>


> Signed-off-by: Ilya Dryomov <ilya.dryomov@inktank.com>
> ---
>  net/ceph/osdmap.c |   53 +++++++++++++++++++++++++++--------------------------
>  1 file changed, 27 insertions(+), 26 deletions(-)
> 
> diff --git a/net/ceph/osdmap.c b/net/ceph/osdmap.c
> index a82df6ea0749..298d076eee89 100644
> --- a/net/ceph/osdmap.c
> +++ b/net/ceph/osdmap.c
> @@ -688,36 +688,37 @@ static int osdmap_decode(void **p, void *end, struct ceph_osdmap *map)
>  {
>  	u16 version;
>  	u32 len, max, i;
> -	int err = -EINVAL;
>  	u32 epoch = 0;
>  	void *start = *p;
> +	int err;
>  	struct ceph_pg_pool_info *pi;
>  
>  	dout("%s %p to %p len %d\n", __func__, *p, end, (int)(end - *p));
>  
> -	ceph_decode_16_safe(p, end, version, bad);
> +	ceph_decode_16_safe(p, end, version, e_inval);
>  	if (version > 6) {
>  		pr_warning("got unknown v %d > 6 of osdmap\n", version);
> -		goto bad;
> +		goto e_inval;
>  	}
>  	if (version < 6) {
>  		pr_warning("got old v %d < 6 of osdmap\n", version);
> -		goto bad;
> +		goto e_inval;
>  	}
>  
> -	ceph_decode_need(p, end, 2*sizeof(u64)+6*sizeof(u32), bad);
> +	ceph_decode_need(p, end, 2*sizeof(u64)+6*sizeof(u32), e_inval);
>  	ceph_decode_copy(p, &map->fsid, sizeof(map->fsid));
>  	epoch = map->epoch = ceph_decode_32(p);
>  	ceph_decode_copy(p, &map->created, sizeof(map->created));
>  	ceph_decode_copy(p, &map->modified, sizeof(map->modified));
>  
> -	ceph_decode_32_safe(p, end, max, bad);
> +	ceph_decode_32_safe(p, end, max, e_inval);
>  	while (max--) {
> -		ceph_decode_need(p, end, 8 + 2, bad);
> -		err = -ENOMEM;
> +		ceph_decode_need(p, end, 8 + 2, e_inval);
>  		pi = kzalloc(sizeof(*pi), GFP_NOFS);
> -		if (!pi)
> +		if (!pi) {
> +			err = -ENOMEM;
>  			goto bad;
> +		}
>  		pi->id = ceph_decode_64(p);
>  		err = __decode_pool(p, end, pi);
>  		if (err < 0) {
> @@ -728,27 +729,25 @@ static int osdmap_decode(void **p, void *end, struct ceph_osdmap *map)
>  	}
>  
>  	err = __decode_pool_names(p, end, map);
> -	if (err < 0) {
> -		dout("fail to decode pool names");
> +	if (err)
>  		goto bad;
> -	}
>  
> -	ceph_decode_32_safe(p, end, map->pool_max, bad);
> +	ceph_decode_32_safe(p, end, map->pool_max, e_inval);
>  
> -	ceph_decode_32_safe(p, end, map->flags, bad);
> +	ceph_decode_32_safe(p, end, map->flags, e_inval);
>  
>  	max = ceph_decode_32(p);
>  
>  	/* (re)alloc osd arrays */
>  	err = osdmap_set_max_osd(map, max);
> -	if (err < 0)
> +	if (err)
>  		goto bad;
>  
>  	/* osds */
> -	err = -EINVAL;
>  	ceph_decode_need(p, end, 3*sizeof(u32) +
>  			 map->max_osd*(1 + sizeof(*map->osd_weight) +
> -				       sizeof(*map->osd_addr)), bad);
> +				       sizeof(*map->osd_addr)), e_inval);
> +
>  	*p += 4; /* skip length field (should match max) */
>  	ceph_decode_copy(p, map->osd_state, map->max_osd);
>  
> @@ -762,7 +761,7 @@ static int osdmap_decode(void **p, void *end, struct ceph_osdmap *map)
>  		ceph_decode_addr(&map->osd_addr[i]);
>  
>  	/* pg_temp */
> -	ceph_decode_32_safe(p, end, len, bad);
> +	ceph_decode_32_safe(p, end, len, e_inval);
>  	for (i = 0; i < len; i++) {
>  		int n, j;
>  		struct ceph_pg pgid;
> @@ -771,16 +770,16 @@ static int osdmap_decode(void **p, void *end, struct ceph_osdmap *map)
>  		err = ceph_decode_pgid(p, end, &pgid);
>  		if (err)
>  			goto bad;
> -		ceph_decode_need(p, end, sizeof(u32), bad);
> +		ceph_decode_need(p, end, sizeof(u32), e_inval);
>  		n = ceph_decode_32(p);
> -		err = -EINVAL;
>  		if (n > (UINT_MAX - sizeof(*pg)) / sizeof(u32))
> -			goto bad;
> -		ceph_decode_need(p, end, n * sizeof(u32), bad);
> -		err = -ENOMEM;
> +			goto e_inval;
> +		ceph_decode_need(p, end, n * sizeof(u32), e_inval);
>  		pg = kmalloc(sizeof(*pg) + n*sizeof(u32), GFP_NOFS);
> -		if (!pg)
> +		if (!pg) {
> +			err = -ENOMEM;
>  			goto bad;
> +		}
>  		pg->pgid = pgid;
>  		pg->len = n;
>  		for (j = 0; j < n; j++)
> @@ -794,10 +793,10 @@ static int osdmap_decode(void **p, void *end, struct ceph_osdmap *map)
>  	}
>  
>  	/* crush */
> -	ceph_decode_32_safe(p, end, len, bad);
> +	ceph_decode_32_safe(p, end, len, e_inval);
>  	dout("osdmap_decode crush len %d from off 0x%x\n", len,
>  	     (int)(*p - start));
> -	ceph_decode_need(p, end, len, bad);
> +	ceph_decode_need(p, end, len, e_inval);
>  	map->crush = crush_decode(*p, end);
>  	*p += len;
>  	if (IS_ERR(map->crush)) {
> @@ -812,6 +811,8 @@ static int osdmap_decode(void **p, void *end, struct ceph_osdmap *map)
>  	dout("full osdmap epoch %d max_osd %d\n", map->epoch, map->max_osd);
>  	return 0;
>  
> +e_inval:
> +	err = -EINVAL;
>  bad:
>  	pr_err("corrupt full osdmap (%d) epoch %d off %d (%p of %p-%p)\n",
>  	       err, epoch, (int)(*p - start), *p, start, end);
>