| Message ID | 1395944299-21970-7-git-send-email-ilya.dryomov@inktank.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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); > -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Mar 27, 2014 at 9:25 PM, Alex Elder <elder@ieee.org> wrote: > 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. Here: err = __decode_pool_names(p, end, map); <-- if (err < 0) { dout("fail to decode pool names"); goto bad; } ceph_decode_32_safe(p, end, map->pool_max, bad); <-- ceph_decode_32_safe(p, end, map->flags, bad); <-- Or here (if at least one pg_temp mapping is present): err = __insert_pg_mapping(pg, &map->pg_temp); <-- if (err) goto bad; dout(" added pg_temp %lld.%x len %d\n", pgid.pool, pgid.seed, len); } /* crush */ ceph_decode_32_safe(p, end, len, bad); <-- dout("osdmap_decode crush len %d from off 0x%x\n", len, (int)(*p - start)); ceph_decode_need(p, end, len, bad); <-- And a lot more in osdmap_apply_incremental(). There are three ways out: (1) a separate variable for helper retvals; (2) resetting err to -EINVAL prior to each ceph_decode_* (if it's not already -EINVAL, of course); (3) a separate e_inval label. (3) is the only reasonable way to do this. (1) leads to things like ret = foo_helper(); if (ret) { err = ret; ... } and (2) is error-prone and hardly maintainable. > > I'd use "einval" or "err_inval" instead of "e_inval". But > no matter. We already use "e_inval" in osd_client.c (OK, that was me), so I'll keep it for consistency. I could rename them all to "Einval" to make them stand out though (I see some "Efoo" labels in fs/). Thanks, Ilya -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/28/2014 09:56 AM, Ilya Dryomov wrote: > On Thu, Mar 27, 2014 at 9:25 PM, Alex Elder <elder@ieee.org> wrote: >> 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. > > Here: > > err = __decode_pool_names(p, end, map); <-- > if (err < 0) { > dout("fail to decode pool names"); > goto bad; > } > > ceph_decode_32_safe(p, end, map->pool_max, bad); <-- > > ceph_decode_32_safe(p, end, map->flags, bad); <-- Fragile indeed. I don't particularly like the encoding of an assumed label in a macro the way these *_safe() macros do either but they do the job and they're all over the place. The biggest reason is that it assumes something about context, but this is another one, it makes things less obvious. Oh well. > Or here (if at least one pg_temp mapping is present): > > err = __insert_pg_mapping(pg, &map->pg_temp); <-- > if (err) > goto bad; > dout(" added pg_temp %lld.%x len %d\n", pgid.pool, pgid.seed, > len); > } > > /* crush */ > ceph_decode_32_safe(p, end, len, bad); <-- > dout("osdmap_decode crush len %d from off 0x%x\n", len, > (int)(*p - start)); > ceph_decode_need(p, end, len, bad); <-- > > And a lot more in osdmap_apply_incremental(). There are three ways out: > > (1) a separate variable for helper retvals; > (2) resetting err to -EINVAL prior to each ceph_decode_* (if it's > not already -EINVAL, of course); > (3) a separate e_inval label. > > (3) is the only reasonable way to do this. (1) leads to things like > > ret = foo_helper(); > if (ret) { > err = ret; > ... > } > > and (2) is error-prone and hardly maintainable. I agree, (3) is the right fix. >> I'd use "einval" or "err_inval" instead of "e_inval". But >> no matter. > > We already use "e_inval" in osd_client.c (OK, that was me), so I'll > keep it for consistency. I could rename them all to "Einval" to make > them stand out though (I see some "Efoo" labels in fs/). Not a big deal. Beauty is in the eye of the beholder. -Alex > Thanks, > > Ilya > -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);
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. Signed-off-by: Ilya Dryomov <ilya.dryomov@inktank.com> --- net/ceph/osdmap.c | 53 +++++++++++++++++++++++++++-------------------------- 1 file changed, 27 insertions(+), 26 deletions(-)