On Thu, Mar 27, 2014 at 9:25 PM, Alex Elder <el...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html