[2/4] batctl: Integrate hardif setting framework
Commit Message
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(-)
Comments
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
@@ -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;
@@ -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;
@@ -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);
@@ -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