On Thu, Sep 27, 2018 at 10:12:09AM +0200, Johannes Berg wrote:
> On Thu, 2018-09-27 at 09:16 +0200, Michal Kubecek wrote:
>
> > The overloading still feels a bit complicated. Perhaps we could rather
> > use validation_data in the natural way, i.e. as a pointer to validation
> > data. That would be a struct (maybe array) of two values of the
> > corresponding type. It would mean a bit more data and a bit more writing
> > but it would be IMHO more straightforward.
>
> I considered that, but I didn't really like it either. The memory
> wasting isn't *that* bad (even if we go to s64 that'd only be ~20x16
> bytes for nl80211, eating up 320 out of the 550 saved, but still); I'm
> more worried about making this really hard to actually *do*.
>
> Consider
>
> policy[] = {
> ...
> [NL80211_ATTR_WIPHY_RETRY_SHORT] =
> NLA_POLICY_RANGE(NLA_U8, 1, 255),
> ...
> };
>
> vs.
>
> static const struct netlink_policy_range retry_range = {
> .min = 1,
> .max = 255,
> };
We could still use helper macros so this part could become
DEFINE_NLA_U8_RANGE(retry_range, 1, 255);
or
DEFINE_NLA_RANGE(retry_range, u8, 1, 255);
>
> policy[] = {
> ...
> [NL80211_ATTR_WIPHY_RETRY_SHORT] = {
> .type = NLA_U8,
> .validation_data = &retry_range,
> },
> ...
> };
And this could be also shortened using a macro.
It would still be longer but not that much.
> That's significantly more to type, to the point where I'd seriously
> consider doing this only for attributes that are used and checked in
> many places - it doesn't feel like a big win over manual range-checking.
>
> But I want it to be a win over manual range-checking so it gets used
> more because it's more efficient, less prone to getting messed up if
> multiple places use the same attribute and validates attributes even if
> they're ignored by an operation.
>
>
> I'd also say that we're certainly no strangers to union/overloading, so
> I don't feel like this is a big argument. One doesn't even really have
> to be *aware* of it for the most part: if it were a struct instead of a
> union, it'd actually have the same effect since the .type field
> indicates which part gets used. That it's overloaded in a union is
> basically just a space saving measure, I don't think it makes the
> reasoning much more complex?
I didn't mean it as a serious objection, rather a note that the gain may
not be worth the additional complexity. But if you want to follow in the
direction you indicated later (in particular, allowing different
interpretations of validation_data for the same type), overloading does
indeed make more sense.
Michal Kubecek
