On 03/28/2014 09:56 AM, Ilya Dryomov wrote:
> 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); <--
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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
