Re: [PATCH net-next RFC 1/1] net netlink: Add new type NLA_FLAG_BITS

[Jamal Hadi Salim <jhs@mojatatu.com>]

Ping.. Following up

On 17-05-02 09:52 PM, Jamal Hadi Salim wrote:
> On 17-05-02 03:03 PM, David Miller wrote:
>> From: Jamal Hadi Salim <jhs@mojatatu.com>
>> Date: Sun, 30 Apr 2017 10:28:39 -0400
>>
>>> Generic bitflags attribute content sent to the kernel by user.
>>> With this type the user can either set or unset a flag in the
>>> kernel.
>>
>> You asked for feedback, here it is :-)
>>
>> I think this is overengineered.
>>
>> Just define a u32 for the value, and mask which defines which bits are
>> legitimate and defined.  Any bit outside of the legitimate mask must
>> be zero.
>>
> 
> That is what it does but as a nla type. It has a validator ops() in
> case someone wants to override the default validator.
> Is the ops the over-engineering you refer to or merely making it an
> nla type is a problem?
>

Since its been a month, a reminder of what the patch introduced a
new netlink type, NLA_FLAG_BITS

UAPI structure:

struct nla_bit_flags {
        u32 nla_flag_values;
        u32 nla_flag_selector;
}

User to kernel example:
---
     nla_flag_values = 0x0, and nla_flag_selector = 0x1
     implies we are selecting bit 1 and we want to set its value to 0.

     nla_flag_values = 0x2, and nla_flag_selector = 0x2
     implies we are selecting bit 2 and we want to set its value to 1.
---

A kernel subsystem specifies validation rules of the following
nature:
----
  [ATTR_GOO] = { .type = NLA_FLAG_BITS,
                 .validation_data = &myvalidflags },

where myvalidflags is the bit mask of the flags the kernel understands.
We reject any bitmask of values that dont fit myvalidflags.

The user can also specify their own validation callback as so:

     [ATTR_GOO] = { .type = MYTYPE,
                    .validation_data = &myvalidation_data,
                    .validate_content = mycontent_validator },

     myvalidation_data is the allowed bitmap as before
     With validator callback looking like:

     int mycontent_validator(const struct nlattr *nla, void *valid_data)
     {
            const struct myattribute *user_data = nla_data(nla);
            struct myvalidation_struct *valid_data_constraint = valid_data;

           ... validate user_data against valid_data_constraint ...
           ... return appropriate error code etc ...
     }
-------

So Dave ;-> Are you suggesting I get rid of the validation op?

cheers,
jamal