Re: [PATCH netifd] bridge: rename "ifname" attribute to "ports"
On 15.05.2021 22:39, Perry wrote:
On 5/15/21 7:18 PM, Rafał Miłecki wrote:
On 15.05.2021 11:09, Paul Oranje wrote:
Op 14 mei 2021, om 15:27 heeft Rafał Miłecki het
volgende geschreven:
From: Rafał Miłecki
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'>
You're most likely confused by the old way of defining bridges that
OpenWrt happens to use by default. It's the UCI "interface" section
type that defines L2 and L3 at the same time.
That said it's not something this patch is trying to address. What gets
you confused I'm planning to rework during next days. The first step
was:
[PATCH] base-files: use "ports" array in board.json network for bridges
https://patchwork.ozlabs.org/project/openwrt/patch/20210514092147.30666-1-zaj...@gmail.com/
The new way of defining L2 in L3 /etc/config/netowrk uses "device" and
"interface" sections. So that would be actually something like:
config device
option name 'lan'
option type 'bridge'
list ports 'lan1'
list ports 'lan2'
On top of that you could then have L3 specified, e.g.:
config interface 'home'
option ifname 'lan'
option proto 'static'
option ipaddr '192.168.1.1'
option netmask '255.255.255.0'
("home" being logical name)
In this example, would a wireless wifi-iface have
option network 'lan' # bridge device name?
or
option network 'home' # logical name?
Since the wifi-iface is added as another port on a bride, then I would
say 'lan'. But on the other hand, a wifi-iface does not have to be
bridged to any physical ports. In this case, I would say 'home' is correct.
You need to use logical name ("home" in above example) in the "network" option.
Please note this is not something this patch affects at all. So what
you're discussing there is out of the scope.
As for the "network" option of wifi-iface I'm planning to rework it
after cleaning up /etc/config/network . Right now it's unpredictable,
see also Jo's comment:
[2021-05-06] [16:24:11 CEST] and not some "just use
wireless.wifi-iface.network" crap which does different things depending on how the
destination network is defined
[2021-05-06] [16:25:07 CEST] (is it a bridge? then bridge it. is it a
vlan? maybe a vlan on top of a bridge, then bridge it too? is it a netdev? then just
inherit ip settings. is it a netdev already in use by something else? then overwrite
random ip settings or simply do nothing, depending on the phase of the moon)
___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Re: [PATCH netifd] bridge: rename "ifname" attribute to "ports"
On 5/15/21 7:18 PM, Rafał Miłecki wrote:
> On 15.05.2021 11:09, Paul Oranje wrote:
>> Op 14 mei 2021, om 15:27 heeft Rafał Miłecki het
>> volgende geschreven:
>>>
>>> From: Rafał Miłecki
>>>
>>> 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'>
> You're most likely confused by the old way of defining bridges that
> OpenWrt happens to use by default. It's the UCI "interface" section
> type that defines L2 and L3 at the same time.
>
> That said it's not something this patch is trying to address. What gets
> you confused I'm planning to rework during next days. The first step
> was:
> [PATCH] base-files: use "ports" array in board.json network for bridges
> https://patchwork.ozlabs.org/project/openwrt/patch/20210514092147.30666-1-zaj...@gmail.com/
>
>
> The new way of defining L2 in L3 /etc/config/netowrk uses "device" and
> "interface" sections. So that would be actually something like:
>
> config device
> option name 'lan'
> option type 'bridge'
> list ports 'lan1'
> list ports 'lan2'
>
> On top of that you could then have L3 specified, e.g.:
>
> config interface 'home'
> option ifname 'lan'
> option proto 'static'
> option ipaddr '192.168.1.1'
> option netmask '255.255.255.0'
>
> ("home" being logical name)
In this example, would a wireless wifi-iface have
option network 'lan' # bridge device name?
or
option network 'home' # logical name?
Since the wifi-iface is added as another port on a bride, then I would
say 'lan'. But on the other hand, a wifi-iface does not have to be
bridged to any physical ports. In this case, I would say 'home' is correct.
>
> ___
> openwrt-devel mailing list
> openwrt-devel@lists.openwrt.org
> https://lists.openwrt.org/mailman/listinfo/openwrt-devel
___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Re: [PATCH netifd] bridge: rename "ifname" attribute to "ports"
On 15.05.2021 11:09, Paul Oranje wrote:
Op 14 mei 2021, om 15:27 heeft Rafał Miłecki het volgende
geschreven:
From: Rafał Miłecki
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'
You're most likely confused by the old way of defining bridges that
OpenWrt happens to use by default. It's the UCI "interface" section
type that defines L2 and L3 at the same time.
That said it's not something this patch is trying to address. What gets
you confused I'm planning to rework during next days. The first step
was:
[PATCH] base-files: use "ports" array in board.json network for bridges
https://patchwork.ozlabs.org/project/openwrt/patch/20210514092147.30666-1-zaj...@gmail.com/
The new way of defining L2 in L3 /etc/config/netowrk uses "device" and
"interface" sections. So that would be actually something like:
config device
option name 'lan'
option type 'bridge'
list ports 'lan1'
list ports 'lan2'
On top of that you could then have L3 specified, e.g.:
config interface 'home'
option ifname 'lan'
option proto 'static'
option ipaddr '192.168.1.1'
option netmask '255.255.255.0'
("home" being logical name)
___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Re: [PATCH netifd] bridge: rename "ifname" attribute to "ports"
The sender domain has a DMARC Reject/Quarantine policy which disallows
sending mailing list messages using the original "From" header.
To mitigate this problem, the original message has been wrapped
automatically by the mailing list software.--- Begin Message ---
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 het volgende
geschreven:
>
> From: Rafał Miłecki
>
> 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
> ---
> 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(>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, _attr_list, );
> - 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, );
> }
>
> +/**
> + * 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(, "name", name);
>
> + config_fixup_bridge_ports(s);
> config_fixup_bridge_vlan_filtering(s, name);
> uci_to_blob(,
Re: [PATCH netifd] bridge: rename "ifname" attribute to "ports"
The sender domain has a DMARC Reject/Quarantine policy which disallows
sending mailing list messages using the original "From" header.
To mitigate this problem, the original message has been wrapped
automatically by the mailing list software.--- Begin Message ---
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 het volgende
geschreven:
>
> From: Rafał Miłecki
>
> 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
> ---
> 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(>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, _attr_list, );
> - 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, );
> }
>
> +/**
> + * 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(, "name", name);
>
> + config_fixup_bridge_ports(s);
> config_fixup_bridge_vlan_filtering(s, name);
> uci_to_blob(,
