From: Alex Elder <elder@ieee.org>
To: Ilya Dryomov <ilya.dryomov@inktank.com>, ceph-devel@vger.kernel.org
Subject: Re: [PATCH 10/33] libceph: fixup error handling in osdmap_apply_incremental()
Date: Thu, 27 Mar 2014 14:49:32 -0500 [thread overview]
Message-ID: <533480CC.5060707@ieee.org> (raw)
In-Reply-To: <1395944299-21970-11-git-send-email-ilya.dryomov@inktank.com>
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. Follow osdmap_decode() and fix this by adding
> a special e_inval label to be used by all ceph_decode_* macros.
Same comments as last time. Otherwise, looks good.
Reviewed-by: Alex Elder <elder@linaro.org>
> Signed-off-by: Ilya Dryomov <ilya.dryomov@inktank.com>
> ---
> net/ceph/osdmap.c | 66 +++++++++++++++++++++++++++--------------------------
> 1 file changed, 34 insertions(+), 32 deletions(-)
>
> diff --git a/net/ceph/osdmap.c b/net/ceph/osdmap.c
> index b70357adbdc0..0fc29a930c06 100644
> --- a/net/ceph/osdmap.c
> +++ b/net/ceph/osdmap.c
> @@ -861,19 +861,19 @@ struct ceph_osdmap *osdmap_apply_incremental(void **p, void *end,
> __s64 new_pool_max;
> __s32 new_flags, max;
> void *start = *p;
> - int err = -EINVAL;
> + int err;
> u16 version;
>
> 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 inc osdmap\n", version);
> - goto bad;
> + goto e_inval;
> }
>
> ceph_decode_need(p, end, sizeof(fsid)+sizeof(modified)+2*sizeof(u32),
> - bad);
> + e_inval);
> ceph_decode_copy(p, &fsid, sizeof(fsid));
> epoch = ceph_decode_32(p);
> BUG_ON(epoch != map->epoch+1);
> @@ -882,7 +882,7 @@ struct ceph_osdmap *osdmap_apply_incremental(void **p, void *end,
> new_flags = ceph_decode_32(p);
>
> /* full map? */
> - ceph_decode_32_safe(p, end, len, bad);
> + ceph_decode_32_safe(p, end, len, e_inval);
> if (len > 0) {
> dout("apply_incremental full map len %d, %p to %p\n",
> len, *p, end);
> @@ -890,13 +890,14 @@ struct ceph_osdmap *osdmap_apply_incremental(void **p, void *end,
> }
>
> /* new crush? */
> - ceph_decode_32_safe(p, end, len, bad);
> + ceph_decode_32_safe(p, end, len, e_inval);
> if (len > 0) {
> - dout("apply_incremental new crush map len %d, %p to %p\n",
> - len, *p, end);
> newcrush = crush_decode(*p, min(*p+len, end));
> - if (IS_ERR(newcrush))
> - return ERR_CAST(newcrush);
> + if (IS_ERR(newcrush)) {
> + err = PTR_ERR(newcrush);
> + newcrush = NULL;
> + goto bad;
> + }
> *p += len;
> }
>
> @@ -906,13 +907,13 @@ struct ceph_osdmap *osdmap_apply_incremental(void **p, void *end,
> if (new_pool_max >= 0)
> map->pool_max = new_pool_max;
>
> - ceph_decode_need(p, end, 5*sizeof(u32), bad);
> + ceph_decode_need(p, end, 5*sizeof(u32), e_inval);
>
> /* new max? */
> max = ceph_decode_32(p);
> if (max >= 0) {
> err = osdmap_set_max_osd(map, max);
> - if (err < 0)
> + if (err)
> goto bad;
> }
>
> @@ -926,11 +927,11 @@ struct ceph_osdmap *osdmap_apply_incremental(void **p, void *end,
> }
>
> /* new_pool */
> - ceph_decode_32_safe(p, end, len, bad);
> + ceph_decode_32_safe(p, end, len, e_inval);
> while (len--) {
> struct ceph_pg_pool_info *pi;
>
> - ceph_decode_64_safe(p, end, pool, bad);
> + ceph_decode_64_safe(p, end, pool, e_inval);
> pi = __lookup_pg_pool(&map->pg_pools, pool);
> if (!pi) {
> pi = kzalloc(sizeof(*pi), GFP_NOFS);
> @@ -947,29 +948,28 @@ struct ceph_osdmap *osdmap_apply_incremental(void **p, void *end,
> }
> if (version >= 5) {
> err = __decode_pool_names(p, end, map);
> - if (err < 0)
> + if (err)
> goto bad;
> }
>
> /* old_pool */
> - ceph_decode_32_safe(p, end, len, bad);
> + ceph_decode_32_safe(p, end, len, e_inval);
> while (len--) {
> struct ceph_pg_pool_info *pi;
>
> - ceph_decode_64_safe(p, end, pool, bad);
> + ceph_decode_64_safe(p, end, pool, e_inval);
> pi = __lookup_pg_pool(&map->pg_pools, pool);
> if (pi)
> __remove_pg_pool(&map->pg_pools, pi);
> }
>
> /* new_up */
> - err = -EINVAL;
> - ceph_decode_32_safe(p, end, len, bad);
> + ceph_decode_32_safe(p, end, len, e_inval);
> while (len--) {
> u32 osd;
> struct ceph_entity_addr addr;
> - ceph_decode_32_safe(p, end, osd, bad);
> - ceph_decode_copy_safe(p, end, &addr, sizeof(addr), bad);
> + ceph_decode_32_safe(p, end, osd, e_inval);
> + ceph_decode_copy_safe(p, end, &addr, sizeof(addr), e_inval);
> ceph_decode_addr(&addr);
> pr_info("osd%d up\n", osd);
> BUG_ON(osd >= map->max_osd);
> @@ -978,11 +978,11 @@ struct ceph_osdmap *osdmap_apply_incremental(void **p, void *end,
> }
>
> /* new_state */
> - ceph_decode_32_safe(p, end, len, bad);
> + ceph_decode_32_safe(p, end, len, e_inval);
> while (len--) {
> u32 osd;
> u8 xorstate;
> - ceph_decode_32_safe(p, end, osd, bad);
> + ceph_decode_32_safe(p, end, osd, e_inval);
> xorstate = **(u8 **)p;
> (*p)++; /* clean flag */
> if (xorstate == 0)
> @@ -994,10 +994,10 @@ struct ceph_osdmap *osdmap_apply_incremental(void **p, void *end,
> }
>
> /* new_weight */
> - ceph_decode_32_safe(p, end, len, bad);
> + ceph_decode_32_safe(p, end, len, e_inval);
> while (len--) {
> u32 osd, off;
> - ceph_decode_need(p, end, sizeof(u32)*2, bad);
> + ceph_decode_need(p, end, sizeof(u32)*2, e_inval);
> osd = ceph_decode_32(p);
> off = ceph_decode_32(p);
> pr_info("osd%d weight 0x%x %s\n", osd, off,
> @@ -1008,7 +1008,7 @@ struct ceph_osdmap *osdmap_apply_incremental(void **p, void *end,
> }
>
> /* new_pg_temp */
> - ceph_decode_32_safe(p, end, len, bad);
> + ceph_decode_32_safe(p, end, len, e_inval);
> while (len--) {
> struct ceph_pg_mapping *pg;
> int j;
> @@ -1018,22 +1018,22 @@ struct ceph_osdmap *osdmap_apply_incremental(void **p, void *end,
> 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);
> pglen = ceph_decode_32(p);
> if (pglen) {
> - ceph_decode_need(p, end, pglen*sizeof(u32), bad);
> + ceph_decode_need(p, end, pglen*sizeof(u32), e_inval);
>
> /* removing existing (if any) */
> (void) __remove_pg_mapping(&map->pg_temp, pgid);
>
> /* insert */
> - err = -EINVAL;
> if (pglen > (UINT_MAX - sizeof(*pg)) / sizeof(u32))
> - goto bad;
> - err = -ENOMEM;
> + goto e_inval;
> pg = kmalloc(sizeof(*pg) + sizeof(u32)*pglen, GFP_NOFS);
> - if (!pg)
> + if (!pg) {
> + err = -ENOMEM;
> goto bad;
> + }
> pg->pgid = pgid;
> pg->len = pglen;
> for (j = 0; j < pglen; j++)
> @@ -1057,6 +1057,8 @@ struct ceph_osdmap *osdmap_apply_incremental(void **p, void *end,
> dout("inc osdmap epoch %d max_osd %d\n", map->epoch, map->max_osd);
> return map;
>
> +e_inval:
> + err = -EINVAL;
> bad:
> pr_err("corrupt inc osdmap (%d) epoch %d off %d (%p of %p-%p)\n",
> err, epoch, (int)(*p - start), *p, start, end);
>
next prev parent reply other threads:[~2014-03-27 19:49 UTC|newest]
Thread overview: 77+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-03-27 18:17 [PATCH 00/33] OSDMAP_ENC, primary_temp, PRIMARY_AFFINITY Ilya Dryomov
2014-03-27 18:17 ` [PATCH 01/33] libceph: refer to osdmap directly in osdmap_show() Ilya Dryomov
2014-03-27 19:09 ` Alex Elder
2014-03-27 18:17 ` [PATCH 02/33] libceph: do not prefix osd lines with \t in debugfs output Ilya Dryomov
2014-03-27 19:10 ` Alex Elder
2014-03-27 18:17 ` [PATCH 03/33] libceph: dump pg_temp mappings to debugfs Ilya Dryomov
2014-03-27 19:11 ` Alex Elder
2014-03-27 18:17 ` [PATCH 04/33] libceph: dump osdmap and enhance output on decode errors Ilya Dryomov
2014-03-27 19:15 ` Alex Elder
2014-03-27 18:17 ` [PATCH 05/33] libceph: split osdmap allocation and decode steps Ilya Dryomov
2014-03-27 19:18 ` Alex Elder
2014-03-27 18:17 ` [PATCH 06/33] libceph: fixup error handling in osdmap_decode() Ilya Dryomov
2014-03-27 19:25 ` Alex Elder
2014-03-28 14:56 ` Ilya Dryomov
2014-03-28 16:22 ` Alex Elder
2014-03-27 18:17 ` [PATCH 07/33] libceph: safely decode max_osd value " Ilya Dryomov
2014-03-27 19:27 ` Alex Elder
2014-03-27 18:17 ` [PATCH 08/33] libceph: assert length of osdmap osd arrays Ilya Dryomov
2014-03-27 19:30 ` Alex Elder
2014-03-28 14:57 ` Ilya Dryomov
2014-03-27 18:17 ` [PATCH 09/33] libceph: fix crush_decode() call site in osdmap_decode() Ilya Dryomov
2014-03-27 19:45 ` Alex Elder
2014-03-28 14:57 ` Ilya Dryomov
2014-03-27 18:17 ` [PATCH 10/33] libceph: fixup error handling in osdmap_apply_incremental() Ilya Dryomov
2014-03-27 19:49 ` Alex Elder [this message]
2014-03-28 14:58 ` Ilya Dryomov
2014-03-27 18:17 ` [PATCH 11/33] libceph: nuke bogus encoding version check " Ilya Dryomov
2014-03-27 19:50 ` Alex Elder
2014-03-27 18:17 ` [PATCH 12/33] libceph: fix and clarify ceph_decode_need() sizes Ilya Dryomov
2014-03-27 19:53 ` Alex Elder
2014-03-27 18:17 ` [PATCH 13/33] libceph: rename __decode_pool{,_names}() to decode_pool{,_names}() Ilya Dryomov
2014-03-27 19:54 ` Alex Elder
2014-03-27 18:18 ` [PATCH 14/33] libceph: introduce decode{,_new}_pools() and switch to them Ilya Dryomov
2014-03-27 19:56 ` Alex Elder
2014-03-27 18:18 ` [PATCH 15/33] libceph: switch osdmap_set_max_osd() to krealloc() Ilya Dryomov
2014-03-27 19:59 ` Alex Elder
2014-03-27 18:18 ` [PATCH 16/33] libceph: introduce decode{,_new}_pg_temp() and switch to them Ilya Dryomov
2014-03-27 20:05 ` Alex Elder
2014-03-27 18:18 ` [PATCH 17/33] libceph: introduce get_osdmap_client_data_v() Ilya Dryomov
2014-03-27 20:17 ` Alex Elder
2014-03-28 14:59 ` Ilya Dryomov
2014-03-27 18:18 ` [PATCH 18/33] libceph: generalize ceph_pg_mapping Ilya Dryomov
2014-03-27 18:18 ` [PATCH 19/33] libceph: primary_temp infrastructure Ilya Dryomov
2014-03-27 20:21 ` Alex Elder
2014-03-27 18:18 ` [PATCH 20/33] libceph: primary_temp decode bits Ilya Dryomov
2014-03-27 20:23 ` Alex Elder
2014-03-27 18:18 ` [PATCH 21/33] libceph: primary_affinity infrastructure Ilya Dryomov
2014-03-27 20:26 ` Alex Elder
2014-03-28 15:01 ` Ilya Dryomov
2014-03-27 18:18 ` [PATCH 22/33] libceph: primary_affinity decode bits Ilya Dryomov
2014-03-27 20:31 ` Alex Elder
2014-03-28 15:01 ` Ilya Dryomov
2014-03-27 18:18 ` [PATCH 23/33] libceph: enable OSDMAP_ENC feature bit Ilya Dryomov
2014-03-27 20:32 ` Alex Elder
2014-03-28 15:01 ` Ilya Dryomov
2014-03-27 18:18 ` [PATCH 24/33] libceph: ceph_osd_{exists,is_up,is_down}(osd) definitions Ilya Dryomov
2014-03-27 20:33 ` Alex Elder
2014-03-27 18:18 ` [PATCH 25/33] libceph: ceph_can_shift_osds(pool) and pool type defines Ilya Dryomov
2014-03-27 20:34 ` Alex Elder
2014-03-27 18:18 ` [PATCH 26/33] libceph: introduce pg_to_raw_osds() and raw_to_up_osds() helpers Ilya Dryomov
2014-03-27 20:36 ` Alex Elder
2014-03-27 18:18 ` [PATCH 27/33] libceph: introduce apply_temps() helper Ilya Dryomov
2014-03-27 20:41 ` Alex Elder
2014-03-27 18:18 ` [PATCH 28/33] libceph: switch ceph_calc_pg_acting() to new helpers Ilya Dryomov
2014-03-27 20:49 ` Alex Elder
2014-03-28 15:02 ` Ilya Dryomov
2014-03-27 18:18 ` [PATCH 29/33] libceph: return primary from ceph_calc_pg_acting() Ilya Dryomov
2014-03-27 20:50 ` Alex Elder
2014-03-27 18:18 ` [PATCH 30/33] libceph: add support for primary_temp mappings Ilya Dryomov
2014-03-27 20:51 ` Alex Elder
2014-03-27 18:18 ` [PATCH 31/33] libceph: add support for osd primary affinity Ilya Dryomov
2014-03-27 20:59 ` Alex Elder
2014-03-28 15:03 ` Ilya Dryomov
2014-03-27 18:18 ` [PATCH 32/33] libceph: redo ceph_calc_pg_primary() in terms of ceph_calc_pg_acting() Ilya Dryomov
2014-03-27 21:04 ` Alex Elder
2014-03-27 18:18 ` [PATCH 33/33] libceph: enable PRIMARY_AFFINITY feature bit Ilya Dryomov
2014-03-27 21:04 ` Alex Elder
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=533480CC.5060707@ieee.org \
--to=elder@ieee.org \
--cc=ceph-devel@vger.kernel.org \
--cc=ilya.dryomov@inktank.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.