| Message ID | 20190613191217.28139-3-sven@narfation.org |
|---|---|
| State | Superseded, archived |
| Delegated to: | Simon Wunderlich |
| Headers | show |
| Series | batctl: Add vid support and hardif settings | expand |
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? 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? 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> in the future, for instance. Regards, Linus
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
diff --git a/main.c b/main.c index 6ca13ac..c806dbf 100644 --- a/main.c +++ b/main.c @@ -35,7 +35,8 @@ static void print_usage(void) { .label = "commands:\n", .types = BIT(SUBCOMMAND) | - BIT(SUBCOMMAND_VID), + BIT(SUBCOMMAND_VID) | + BIT(SUBCOMMAND_HIF), }, { .label = "debug tables: \tdisplay the corresponding debug table\n", @@ -51,6 +52,10 @@ static void print_usage(void) "vid <vid> ", NULL, }; + const char *hardif_prefixes[] = { + "hardif <netdev> ", + NULL, + }; const struct command **p; const char **prefixes; const char **prefix; @@ -81,6 +86,9 @@ static void print_usage(void) case SUBCOMMAND_VID: prefixes = vlan_prefixes; break; + case SUBCOMMAND_HIF: + prefixes = hardif_prefixes; + break; default: prefixes = default_prefixes; break; @@ -133,6 +141,12 @@ static const struct command *find_command(struct state *state, const char *name) if (state->vid < 0 && cmd->type == SUBCOMMAND_VID) continue; + if (state->hif > 0 && cmd->type != SUBCOMMAND_HIF) + continue; + + if (state->hif == 0 && cmd->type == SUBCOMMAND_HIF) + continue; + if (strcmp(cmd->name, name) == 0) return cmd; @@ -180,6 +194,18 @@ static int parse_dev_args(struct state *state, int argc, char *argv[]) state->arg_iface = argv[1]; translate_mesh_iface(state); + return 2; + } else if (strcmp(argv[0], "hardif") == 0) { + state->hif = if_nametoindex(argv[1]); + if (state->hif == 0) { + fprintf(stderr, "Error - hard interface not found\n"); + return -ENODEV; + } + + snprintf(state->hard_iface, sizeof(state->hard_iface), "%s", + argv[1]); + + translate_mesh_iface(state); return 2; } else { /* parse vlan as part of -m parameter */ @@ -193,6 +219,7 @@ int main(int argc, char **argv) const struct command *cmd; struct state state = { .arg_iface = mesh_dfl_iface, + .hif = 0, .cmd = NULL, }; int dev_arguments; diff --git a/main.h b/main.h index 1d95261..a27d848 100644 --- a/main.h +++ b/main.h @@ -59,6 +59,7 @@ enum command_flags { enum command_type { SUBCOMMAND, SUBCOMMAND_VID, + SUBCOMMAND_HIF, DEBUGTABLE, }; @@ -66,6 +67,8 @@ struct state { char *arg_iface; char mesh_iface[IF_NAMESIZE]; unsigned int mesh_ifindex; + char hard_iface[IF_NAMESIZE]; + unsigned int hif; int vid; const struct command *cmd; diff --git a/sys.c b/sys.c index f19719c..fd34b2f 100644 --- a/sys.c +++ b/sys.c @@ -150,6 +150,10 @@ static void settings_usage(struct state *state) "vid <vid> ", NULL, }; + const char *hardif_prefixes[] = { + "hardif <netdev> ", + NULL, + }; const char *linestart = "Usage:"; const char **prefixes; const char **prefix; @@ -158,6 +162,9 @@ static void settings_usage(struct state *state) case SUBCOMMAND_VID: prefixes = vlan_prefixes; break; + case SUBCOMMAND_HIF: + prefixes = hardif_prefixes; + break; default: prefixes = default_prefixes; break; @@ -259,15 +266,23 @@ int handle_sys_setting(struct state *state, int argc, char **argv) return EXIT_FAILURE; } - /* if the specified interface is a VLAN then change the path to point - * to the proper "vlan%{vid}" subfolder in the sysfs tree. - */ - if (state->vid >= 0) + if (state->hif > 0) { + /* if a hard interface was specified then change the path to + * point to the proper ${hardif}/batman-adv path in the sysfs + * tree. + */ + snprintf(path_buff, PATH_BUFF_LEN, SYS_HARDIF_PATH, + state->hard_iface); + } else if (state->vid >= 0) { + /* if the specified interface is a VLAN then change the path to + * point to the proper "vlan%{vid}" subfolder in the sysfs tree. + */ snprintf(path_buff, PATH_BUFF_LEN, SYS_VLAN_PATH, state->mesh_iface, state->vid); - else + } else { snprintf(path_buff, PATH_BUFF_LEN, SYS_BATIF_PATH_FMT, state->mesh_iface); + } if (argc == 1) { res = sys_read_setting(state, path_buff, settings->sysfs_name); diff --git a/sys.h b/sys.h index d4f2fcf..b6f0f90 100644 --- a/sys.h +++ b/sys.h @@ -21,8 +21,9 @@ #define SYS_BATIF_PATH_FMT "/sys/class/net/%s/mesh/" #define SYS_IFACE_PATH "/sys/class/net" #define SYS_IFACE_DIR SYS_IFACE_PATH"/%s/" -#define SYS_MESH_IFACE_FMT SYS_IFACE_PATH"/%s/batman_adv/mesh_iface" -#define SYS_IFACE_STATUS_FMT SYS_IFACE_PATH"/%s/batman_adv/iface_status" +#define SYS_HARDIF_PATH SYS_IFACE_DIR "batman_adv/" +#define SYS_MESH_IFACE_FMT SYS_HARDIF_PATH "mesh_iface" +#define SYS_IFACE_STATUS_FMT SYS_HARDIF_PATH "iface_status" #define SYS_VLAN_PATH SYS_IFACE_PATH"/%s/mesh/vlan%d/" #define SYS_ROUTING_ALGO_FMT SYS_IFACE_PATH"/%s/mesh/routing_algo" #define VLAN_ID_MAX_LEN 4
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> --- main.c | 29 ++++++++++++++++++++++++++++- main.h | 3 +++ sys.c | 25 ++++++++++++++++++++----- sys.h | 5 +++-- 4 files changed, 54 insertions(+), 8 deletions(-)