Re: [PATCH 2/4] batctl: Integrate hardif setting framework

[Sven Eckelmann <sven@narfation.org>]

[-- Attachment #1: Type: text/plain, Size: 6825 bytes --]

On Sunday, 16 June 2019 16:53:16 CEST Linus Lüssing wrote:
> On Thu, Jun 13, 2019 at 09:12:15PM +0200, Sven Eckelmann wrote:
> > batctl currently supports settings which are either mesh interface or vlan
> > specific. But B.A.T.M.A.N. V introduced two additional settings which are
> > hard (slave) interface specific.
> > 
> > To support these, an additional command prefix called hardif is implemented
> > for some sysfs commands:
> > 
> >   $ batctl -m bat0 hardif eth0 ...
> > 
> > Signed-off-by: Sven Eckelmann <sven@narfation.org>
> > ---
> 
> Three thoughts/questions:
> 
> Currently we do not allow adding a hard-interface to two meshes,
> right? So the "-m bat0" here is redundant?

Yes and no. This is also the way how the netlink interface is addressing the 
device. But implementation wise - this should be rather easy. I've already 
added the code to query_rtnl_link_single a while back. See
check_mesh_iface_ownership_netlink as an example.

So it is now a question of how many magic we want to implement at the 
beginning. We already had the problem that the old vlan selection logic (-m) 
could be used to run weird commands which you shouldn't be able to run that 
way. Because of this I would ask to deprecate the '-m' parameter in favor of 
an optional(?) meshif selector prefix. And show this selector prefix for vid 
and for all meshif specific commands.

> Have we used the terminology "hard interface" in UI and
> documentation before? Maybe it's just me, but I'm wondering
> whether the terms "soft interface" and "hard interface" might be a
> bit confusing to users, as these days people not only add
> hardware interfaces but also virtual ones. And these terms are not
> used in other projects (afaik). Maybe just stick to the more commonly
> used term "slave interface" and keep "hard" and "soft" interface as
> internal?

We are using hardif for example in the OpenWrt config integration.
And the netlink stuff is called this way. Also the event parser 
already print events out as "hardif" events (for hardif related events only of 
course).

And yes, this was then also used in the documentation.

> I'm wondering how it would look like if we were having settings
> both applicable to a soft and hard interface. What about using a
> "-s <slave-iface>", similar to the "-m <mesh-iface>" instead of
> the "hardif" command prefix? So that you could do things like:
> 
> $ batctl [-m <mesh-iface>|-s <slave-iface>] multicast_fanout <int>

Just do it like you do it for ap_isolation - which is for both vlan and 
meshif:

   batctl ap_isolation
   batctl vid 1 ap_isolation

Using these selector prefixes instead of -parameter value things allows us 
to correctly filter the commands and to provide an overview of commands with 
the information for which object type it can be used. Something like the stuff
we are doing for ap_isolation with this patchset:

        ap_isolation|ap                    [0|1]                display or modify ap_isolation setting
        vlan <vdev> ap_isolation|ap        [0|1]                display or modify ap_isolation setting for vlan device or id
        vid <vid> ap_isolation|ap          [0|1]                display or modify ap_isolation setting for vlan device or id

And I tend to have problems with -parameters when the order is too important 
and not really clear. For example following would work:

   batctl -m bat0 ping 01:23:45:67:89:af

But not following:

   batctl ping -m bat0 01:23:45:67:89:af 

While you can learn to handle this correctly, it seems to more intuitive to 
have it tree structured from the start. Simply to make it clear on what you
are operating now. Something more like:

    [meshif <dev>]   aggregation|ag              [0|1]                display or modify aggregation setting
    [meshif <dev>]   ap_isolation|ap             [0|1]                display or modify ap_isolation setting
    vlan <vdev>      ap_isolation|ap             [0|1]                display or modify ap_isolation setting for vlan device or id
    [meshif <dev>] vid <vid> ap_isolation|ap     [0|1]                display or modify ap_isolation setting for vlan device or id
    [meshif <dev>]   bonding|b                   [0|1]                display or modify bonding setting
    [meshif <dev>]   bridge_loop_avoidance|bl    [0|1]                display or modify bridge_loop_avoidance setting
    [meshif <dev>]   distributed_arp_table|dat   [0|1]                display or modify distributed_arp_table setting
    hardif <netdev>  elp_interval|et             [interval]           display or modify elp_interval setting
    event|e                                                           display events from batman-adv
    [meshif <dev>]   fragmentation|f             [0|1]                display or modify fragmentation setting
    [meshif <dev>]   gw_mode|gw                  [mode]               display or modify the gateway mode
    [meshif <dev>]   hop_penalty|hp              [penalty]            display or modify hop_penalty setting
    [meshif <dev>]   interface|if                [add|del iface(s)]   display or modify the interface settings
    [meshif <dev>]   isolation_mark|mark         [mark]               display or modify isolation_mark setting
    [meshif <dev>]   loglevel|ll                 [level]              display or modify the log level
    [meshif <dev>]   multicast_fanout|mo         [fanout]             display or modify multicast_fanout setting
    [meshif <dev>]   multicast_forceflood|mff    [0|1]                display or modify multicast_forceflood setting
    [meshif <dev>]   network_coding|nc           [0|1]                display or modify network_coding setting
    [meshif <dev>]   orig_interval|it            [interval]           display or modify orig_interval setting
    [meshif <dev>]   ping|p                      <destination>        ping another batman adv host via layer 2
    routing_algo|ra                              [mode]               display or modify the routing algorithm
    [meshif <dev>]   statistics|s                                     print mesh statistics
    [meshif <dev>]   tcpdump|td                  <interface>          tcpdump layer 2 traffic on the given interface
    hardif <netdev>  throughput_override|to      [mbit]               display or modify throughput_override setting
    [meshif <dev>]   throughputmeter|tp          <destination>        start a throughput measurement
    [meshif <dev>]   traceroute|tr               <destination>        traceroute another batman adv host via layer 2
    [meshif <dev>]   translate|t                 <destination>        translate a destination to the originator responsible for it

Kind regards,
	Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]