[PATCH netifd] bridge: rename "ifname" attribute to "ports"

[Paul Oranje]

Good morning !
Thanks for trying to improve on the model set in the netifd UCI, but ...
Regards, Paul

Op 14 mei 2021, om 15:27 heeft Rafał Miłecki <zajec5 at gmail.com> het volgende geschreven:
> 
> From: Rafał Miłecki <rafal at milecki.pl>
> 
> Bridge aggregates multiple ports so use a more accurate name ("ports").
Confusing that a logical network "interface" references something physical like a "port".
One would expect that at least a level is modelled in between and that a bridge in a "interface" config section can bridge several devices (like plain devices/L2 bridges/VLANs/tunnel devices/...).
Assuming I fail to understand the model, what am I missing ?

> For backward compatibility add a temporary config translation.
> 
> Config example:
> 
> config interface 'lan'
>        option type 'bridge'
>        list ports 'lan1'
>        list ports 'lan2'
> 
> Signed-off-by: Rafał Miłecki <rafal at milecki.pl>
> ---
> bridge.c | 16 ++++++++--------
> config.c | 23 ++++++++++++++++++++++-
> 2 files changed, 30 insertions(+), 9 deletions(-)
> 
> diff --git a/bridge.c b/bridge.c
> index 099dfe4..a1f3992 100644
> --- a/bridge.c
> +++ b/bridge.c
> @@ -23,7 +23,7 @@
> #include "system.h"
> 
> enum {
> -	BRIDGE_ATTR_IFNAME,
> +	BRIDGE_ATTR_PORTS,
> 	BRIDGE_ATTR_STP,
> 	BRIDGE_ATTR_FORWARD_DELAY,
> 	BRIDGE_ATTR_PRIORITY,
> @@ -44,7 +44,7 @@ enum {
> };
> 
> static const struct blobmsg_policy bridge_attrs[__BRIDGE_ATTR_MAX] = {
> -	[BRIDGE_ATTR_IFNAME] = { "ifname", BLOBMSG_TYPE_ARRAY },
> +	[BRIDGE_ATTR_PORTS] = { "ports", BLOBMSG_TYPE_ARRAY },
> 	[BRIDGE_ATTR_STP] = { "stp", BLOBMSG_TYPE_BOOL },
> 	[BRIDGE_ATTR_FORWARD_DELAY] = { "forward_delay", BLOBMSG_TYPE_INT32 },
> 	[BRIDGE_ATTR_PRIORITY] = { "priority", BLOBMSG_TYPE_INT32 },
> @@ -64,7 +64,7 @@ static const struct blobmsg_policy bridge_attrs[__BRIDGE_ATTR_MAX] = {
> };
> 
> static const struct uci_blob_param_info bridge_attr_info[__BRIDGE_ATTR_MAX] = {
> -	[BRIDGE_ATTR_IFNAME] = { .type = BLOBMSG_TYPE_STRING },
> +	[BRIDGE_ATTR_PORTS] = { .type = BLOBMSG_TYPE_STRING },
> };
> 
> static const struct uci_blob_param_list bridge_attr_list = {
> @@ -104,7 +104,7 @@ struct bridge_state {
> 
> 	struct blob_attr *config_data;
> 	struct bridge_config config;
> -	struct blob_attr *ifnames;
> +	struct blob_attr *ports;
> 	bool active;
> 	bool force_active;
> 	bool has_vlans;
> @@ -853,8 +853,8 @@ bridge_config_init(struct device *dev)
> 
> 	bst->n_failed = 0;
> 	vlist_update(&bst->members);
> -	if (bst->ifnames) {
> -		blobmsg_for_each_attr(cur, bst->ifnames, rem) {
> +	if (bst->ports) {
> +		blobmsg_for_each_attr(cur, bst->ports, rem) {
> 			bridge_add_member(bst, blobmsg_data(cur));
> 		}
> 	}
> @@ -970,7 +970,7 @@ bridge_reload(struct device *dev, struct blob_attr *attr)
> 	if (tb_dev[DEV_ATTR_MACADDR])
> 		bst->primary_port = NULL;
> 
> -	bst->ifnames = tb_br[BRIDGE_ATTR_IFNAME];
> +	bst->ports = tb_br[BRIDGE_ATTR_PORTS];
> 	device_init_settings(dev, tb_dev);
> 	bridge_apply_settings(bst, tb_br);
> 
> @@ -991,7 +991,7 @@ bridge_reload(struct device *dev, struct blob_attr *attr)
> 
> 		diff = 0;
> 		uci_blob_diff(tb_br, otb_br, &bridge_attr_list, &diff);
> -		if (diff & ~(1 << BRIDGE_ATTR_IFNAME))
> +		if (diff & ~(1 << BRIDGE_ATTR_PORTS))
> 		    ret = DEV_CONFIG_RESTART;
> 
> 		bridge_config_init(dev);
> diff --git a/config.c b/config.c
> index fa7cbe4..1f4560f 100644
> --- a/config.c
> +++ b/config.c
> @@ -95,6 +95,24 @@ config_fixup_bridge_var(struct uci_section *s, const char *name, const char *val
> 	uci_set(uci_ctx, &ptr);
> }
> 
> +/**
> + * config_fixup_bridge_ports - translate deprecated configs
> + *
> + * Old configs used "ifname" option for specifying bridge ports. For backward
> + * compatibility translate it into the new "ports" option.
> + */
> +static void config_fixup_bridge_ports(struct uci_section *s)
> +{
> +	const char *ifname;
> +
> +	if (uci_lookup_option(uci_ctx, s, "ports"))
> +		return;
> +
> +	ifname = uci_lookup_option_string(uci_ctx, s, "ifname");
> +	if (ifname)
> +		config_fixup_bridge_var(s, "ports", ifname);
> +}
> +
> static void
> config_fixup_bridge_vlan_filtering(struct uci_section *s, const char *name)
> {
> @@ -117,6 +135,7 @@ config_parse_bridge_interface(struct uci_section *s, struct device_type *devtype
> 	sprintf(name, "%s-%s", devtype->name_prefix, s->e.name);
> 	blobmsg_add_string(&b, "name", name);
> 
> +	config_fixup_bridge_ports(s);
> 	config_fixup_bridge_vlan_filtering(s, name);
> 	uci_to_blob(&b, s, devtype->config_params);
> 	if (!device_create(name, devtype, b.head)) {
> @@ -254,8 +273,10 @@ config_init_devices(bool bridge)
> 		if (!params)
> 			params = simple_device_type.config_params;
> 
> -		if (devtype && devtype->bridge_capability)
> +		if (devtype && devtype->bridge_capability) {
> +			config_fixup_bridge_ports(s);
> 			config_fixup_bridge_vlan_filtering(s, name);
> +		}
> 
> 		blob_buf_init(&b, 0);
> 		uci_to_blob(&b, s, params);
> -- 
> 2.26.2
> 
> 
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel at lists.openwrt.org
> https://lists.openwrt.org/mailman/listinfo/openwrt-devel