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