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