Re: [B.A.T.M.A.N.] [RFC 1/2] batman-adv: Add infrastructure for netlink config manipulation

[Jiri Pirko <jiri@mellanox.com>]

Mon, Nov 05, 2018 at 12:33:25PM CET, sven@narfation.org wrote:
>On Montag, 5. November 2018 11:17:08 CET Jiri Pirko wrote:
>> Sun, Nov 04, 2018 at 08:33:13PM CET, sven@narfation.org wrote:
>> >The batman-adv configuration interface was implemented solely using sysfs.
>> >This approach was condemned by non-batadv developers as "huge mistake".
>> >Instead a netlink/genl based implementation was suggested.
>> >
>> >The different key-value pairs used for the configuration of batman-adv can
>> >be split in three classes (sharing most of the setup code):
>> 
>> This is problematic. In general, TLV approach is frowned upon.
>> What is a usual way of exposing options over netlink are well-defined
>> netlink attributes. For example:
>> 
>> BATADV_ATTR_SOME_THING
>> BATADV_ATTR_ANOTHER_THING
>> 
>> The set and get commands usually work with an object. So if and object
>> have multiple options/attributes, it can we set via a single set
>> command. But this is more free. Depends on what makes more sense.
>
>Hm, so you first  say that I should not do that and then say that it can be 
>done because it operates on the same object? Classes in my description are 
>basically your object.

The API needs to be well-defined. That, you can do by defining ATTRs.
But with TVLs, that cannot be be done. API is NAME-VALUE and user does
not really see what "NAME" he can pass down, as it is parsed later on in
batman code. That is my point. To have all options as well-defined
netlink attributes with types (for netlink policy purposes).


>
>But let assume for now that you don't want to use this approach, we can also 
>have for example something like:
>
>BATADV_CMD_SET_BRIDGE_LOOP_AVOIDANCE which then receives either one or no flag 
>BATADV_ATTR_BRIDGE_LOOP_AVOIDANCE to enable/disable bridge loop avoidance. And 
>a command BATADV_CMD_GET_BRIDGE_LOOP_AVOIDANCE which tells the client using 
>BATADV_ATTR_BRIDGE_LOOP_AVOIDANCE whether it was enabled or not. A dump 
>functionality for all options for this batadv object/class would then not 
>exist.

Got it. That is possible to do it this way. But if you would be able to
group the attrs somehow, that would be probably nice. 1:1 mapping
between cmds and attrs looks a bit weird.

Sidenote: I suggest not to use NLA_FLAG attribute type. Better to use
NLA_U8 of value 0/1. NLA_FLAG missing in message might mean 2 things:
1) user wants to set false
2) user is old and does not support this attribute



>
>Kind regards,
>	Sven