Pavel Šimerda <[email protected]> [2021-01-12 04:36:36]: Hi,
> The module requires libteam's teamd and teamdctl commands. This looks like an alien to the OpenWrt ecosystem, basically you're using netifd just as a launcher for teamd, teamdctl without any proper error handling instead of ubus for configuration etc. > Signed-off-by: Pavel Šimerda <[email protected]> > --- > CMakeLists.txt | 2 +- > team.c | 178 +++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 179 insertions(+), 1 deletion(-) > create mode 100644 team.c > > diff --git a/CMakeLists.txt b/CMakeLists.txt > index 9d19817..351e303 100644 > --- a/CMakeLists.txt > +++ b/CMakeLists.txt > @@ -19,7 +19,7 @@ SET(SOURCES > main.c utils.c system.c tunnel.c handler.c > interface.c interface-ip.c interface-event.c > iprule.c proto.c proto-static.c proto-shell.c > - config.c device.c bridge.c veth.c vlan.c alias.c > + config.c device.c bridge.c team.c veth.c vlan.c alias.c > macvlan.c ubus.c vlandev.c wireless.c) > > > diff --git a/team.c b/team.c > new file mode 100644 > index 0000000..9b67566 > --- /dev/null > +++ b/team.c > @@ -0,0 +1,178 @@ > +/* > + * netifd - network interface daemon > + * Copyright (C) 2021 Pavel Šimerda <[email protected]> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 > + * as published by the Free Software Foundation > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > +#include <string.h> > +#include <stdlib.h> > +#include <stdio.h> > +#include <assert.h> > +#include <errno.h> > +#include <signal.h> > +#include <sys/wait.h> > +#include <unistd.h> > + > +#include "netifd.h" > +#include "device.h" > +#include "interface.h" > +#include "system.h" > + > +enum { > + TEAM_ATTR_IFNAME, > + __TEAM_ATTR_MAX > +}; > + > +static const struct blobmsg_policy team_attrs[__TEAM_ATTR_MAX] = { > + [TEAM_ATTR_IFNAME] = { "ifname", BLOBMSG_TYPE_ARRAY }, > +}; > + > +static const struct uci_blob_param_info team_attr_info[__TEAM_ATTR_MAX] = { > + [TEAM_ATTR_IFNAME] = { .type = BLOBMSG_TYPE_STRING }, > +}; > + > +static const struct uci_blob_param_list team_attr_list = { > + .n_params = __TEAM_ATTR_MAX, > + .params = team_attrs, > + .info = team_attr_info, > + > + .n_next = 1, > + .next = { &device_attr_list }, > +}; > + > +struct team_device { > + struct device dev; > + device_state_cb set_state; > + > + struct blob_attr *config_data; > + struct blob_attr *ifnames; > + > + int pid; > +}; > + > +static int > +team_set_state(struct device *dev, bool up) > +{ > + struct team_device *teamdev = container_of(dev, struct team_device, > dev); > + > + if (up) { > + int pid; > + struct blob_attr *cur; > + int rem; > + char buffer[64]; > + > + printf("TEAM start teamd\n"); if this is needed at all, take a look around and use proper debug logging > + pid = fork(); > + if (pid == -1) > + return -errno; > + if (pid == 0) > + execlp("teamd", "teamd", "-t", dev->ifname, NULL); this is external dependency and you lack any check for that > + teamdev->pid = pid; > + // TODO: Better handling of newly created devices. better? there is no handling of anything > + sleep(1); > + if (teamdev->ifnames) { > + printf("TEAM port init\n"); > + blobmsg_for_each_attr(cur, teamdev->ifnames, rem) { > + printf("TEAM one port init\n"); > + snprintf(buffer, sizeof buffer, > + "teamdctl %s port add %s", > + dev->ifname, > + blobmsg_get_string(cur)); > + system(buffer); puting aside usage of system() for service configuration this smells, you're passing possibly malicious input directly to system() in the service running as root, what could go wrong? > + } > + } > + teamdev->set_state(dev, up); > + return 0; > + } else { > + printf("TEAM: killing %d\n", teamdev->pid); > + if (teamdev->pid) { > + kill(teamdev->pid, SIGTERM); > + waitpid(teamdev->pid, NULL, 0); > + teamdev->pid = 0; > + } > + return 0; > + } > +} > + > +static enum dev_change_type > +team_reload(struct device *dev, struct blob_attr *attr) > +{ > + struct team_device *teamdev = container_of(dev, struct team_device, > dev); > + struct blob_attr *tb_tm[__TEAM_ATTR_MAX]; > + > + attr = blob_memdup(attr); > + > + blobmsg_parse(team_attrs, __TEAM_ATTR_MAX, tb_tm, blob_data(attr), > blob_len(attr)); > + teamdev->ifnames = tb_tm[TEAM_ATTR_IFNAME]; > + > + if (teamdev->pid) { > + // TODO: More fine-grained reconfiguration > + team_set_state(dev, false); > + team_set_state(dev, true); > + } > + > + free(teamdev->config_data); > + teamdev->config_data = attr; > + return DEV_CONFIG_APPLIED; > +} > + > +static struct device * > +team_create(const char *name, struct device_type *devtype, > + struct blob_attr *attr) > +{ > + struct team_device *teamdev; > + struct device *dev = NULL; > + > + teamdev = calloc(1, sizeof(*teamdev)); > + if (!teamdev) > + return NULL; > + dev = &teamdev->dev; > + > + if (device_init(dev, devtype, name) < 0) { > + device_cleanup(dev); > + free(teamdev); > + return NULL; > + } > + > + teamdev->set_state = dev->set_state; > + dev->set_state = team_set_state; > + > + device_set_present(dev, true); > + team_reload(dev, attr); > + > + return dev; > +} > + > +static void > +team_free(struct device *dev) > +{ > + struct team_device *teamdev = container_of(dev, struct team_device, > dev); > + > + free(teamdev->config_data); > + free(teamdev); > +} > + > +static struct device_type team_device_type = { > + .name = "team", > + .config_params = &team_attr_list, > + > + .bridge_capability = true, > + .name_prefix = "tm", > + > + .create = team_create, > + .reload = team_reload, > + .free = team_free, > +}; > + > +static void __init team_device_type_init(void) > +{ > + device_type_add(&team_device_type); > +} > -- > 2.29.2 _______________________________________________ openwrt-devel mailing list [email protected] https://lists.openwrt.org/mailman/listinfo/openwrt-devel
