Project

General

Profile

Actions

Feature #300

closed

Convert sysfs config attributes to genl

Added by Sven Eckelmann about 8 years ago. Updated over 4 years ago.

Status:
Closed
Priority:
Normal
Target version:
Start date:
10/29/2016
Due date:
% Done:

100%

Estimated time:
80.00 h

Description

Jiri Pirko is thinking that sysfs use to config attributes of object/batman-adv is "is a huge mistake to use sysfs for things like this." https://patchwork.open-mesh.org/project/b.a.t.m.a.n./patch/20161027190150.7880-4-sw@simonwunderlich.de/

Instead he "suggest to rework the api to use genl entirely."

Actions #1

Updated by Sven Eckelmann about 8 years ago

  • Estimated time set to 40.00 h
Actions #2

Updated by Sven Eckelmann almost 8 years ago

  • Assignee set to batman-adv developers
Actions #3

Updated by Sven Eckelmann about 6 years ago

  • Status changed from New to In Progress
Actions #4

Updated by Sven Eckelmann about 6 years ago

  • Estimated time changed from 40.00 h to 80.00 h

Jiri informed us that this approach is now frowned upon. Instead following implementation is preferred:

On Montag, 5. November 2018 11:54:12 CET Jiri Pirko wrote:
[...]

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

Ok, let me suggest something more specific (just to make sure we talk about
the same thing). We could use the commands:

> +       /**
> +        * @BATADV_CMD_GET_OPTION: Get option(s) from softif
> +        */
> +       BATADV_CMD_GET_OPTION,
> +
> +       /**
> +        * @BATADV_CMD_SET_OPTION: Set option for softif
> +        */
> +       BATADV_CMD_SET_OPTION,
> +
> +       /**
> +        * @BATADV_CMD_GET_OPTION_HARDIF: Get option(s) from a hardif of the
> +        *  current softif
> +        */
> +       BATADV_CMD_GET_OPTION_HARDIF,
> +
> +       /**
> +        * @BATADV_CMD_SET_OPTION_HARDIF: Set option for hardif of the
> +        *  current softif
> +        */
> +       BATADV_CMD_SET_OPTION_HARDIF,
> +
> +       /**
> +        * @BATADV_CMD_GET_OPTION_VLAN: Get option(s) from a VLAN of the
> +        *  current softif
> +        */
> +       BATADV_CMD_GET_OPTION_VLAN,
> +
> +       /**
> +        * @BATADV_CMD_SET_OPTION_VLAN: Set option for VLAN of the
> +        *  current softif
> +        */
> +       BATADV_CMD_SET_OPTION_VLAN,
> +
> 

But instead of using BATADV_ATTR_OPTION_NAME+BATADV_ATTR_OPTION_TYPE+
BATADV_ATTR_OPTION_VALUE to send the infos up, we have a number of specific
netlink attributes [e.g. BATADV_ATTR_BRIDGE_LOOP_AVOIDANCE (u8)] that are used
to exchange the value directly. Multiple attributes can be in the messages
and dumping is possible but maybe requires to split all option in multiple
messages. Informational dumping (user readable) is then only possible when the
userspace part also knows all attributes. And sending things to the kernel is
only possible when the userspace also has explicit support for the attribute.

This is at least similar to how I did it in two previous projects but dropped
this approach when I saw that it was implemented differently for devlink and
team.

We can maybe modify the current way how options are registered to write/read directly to msg/attributes. But the main logic has to be modified anyway. And we cannot support something like the config command.

(A lot) more time has therefore to be spend on this topic before it is ready.

It should be checked whether NL80211_CMD_SET_WIPHY is a good reference.

And also NLA_FLAG should be dropped. Instead:

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

Actions #5

Updated by Sven Eckelmann about 6 years ago

There is now a new patch series for batman-adv https://patchwork.open-mesh.org/project/b.a.t.m.a.n./list/?series=181

The batctl implementation can currently only receive the netlink messages of the "config" mcast group. But at least the GET replies will look the same: https://patchwork.open-mesh.org/project/b.a.t.m.a.n./patch/20181123161442.13461-2-sven@narfation.org/

Actions #7

Updated by Sven Eckelmann almost 6 years ago

The RFC patch series for batman-adv was updated:

The batctl patch series from #300#note-6 can still be used because the ABI didn't change.

Actions #8

Updated by Sven Eckelmann almost 6 years ago

  • Status changed from In Progress to Resolved
  • Target version set to 2019.1
  • % Done changed from 0 to 100
Actions #9

Updated by Sven Eckelmann almost 6 years ago

  • Status changed from Resolved to Closed

Added to net-next and batman-adv master branch.

Actions #10

Updated by Sven Eckelmann over 4 years ago

  • Description updated (diff)
Actions

Also available in: Atom PDF