[PATCH netifd] team: add simple bonding/teaming module

[Pavel Šimerda]

On 1/16/21 1:55 PM, Petr Štetiar wrote:
> Pavel Šimerda <code at simerda.eu> [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.

Hey Petr,

this is what I'm using right now to enable the hardware features that were *absent* in OpenWRT. Would you suggest that we keep a local fork for the time being, until someone is willing to invest their time into building a ubus wrapper for libteam?

Cheers,

Pavel Šimerda

> 
>> Signed-off-by: Pavel Šimerda <code at simerda.eu>
>> ---
>>   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 <code at simerda.eu>
>> + *
>> + * 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