Re: [PATCH netifd] interface: rename "ifname" attribute to "device"
On 2021-05-17 16:49, Rafał Miłecki wrote: > From: Rafał Miłecki > > Interfaces need to be assigned to devices. For that purpose a "device" > option should be more accurate than "ifname" one. > > For backward compatibility add a temporary config translation. > > Config example: > > config device > option name 'lan' > option type 'bridge' > list ports 'lan1' > list ports 'lan2' > > config interface 'home' > option device 'lan' > option proto 'static' > > Signed-off-by: Rafał Miłecki While I agree that the 'device' name would fit better than 'ifname', the fallback config translation is not enough in this case. netifd supports adding dynamic interfaces via ubus, where the config data is provided as blobmsg. You could update the in-tree users of that, but I don't know if there are any out-of-tree downstream users of this functionality that you would be breaking with this change. - Felix ___ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Re: [PATCH netifd] interface: rename "ifname" attribute to "device"
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 --- > Op 17 mei 2021, om 17:47 heeft Rafał Miłecki het volgende > geschreven: > > On 17.05.2021 17:32, Paul Oranje wrote: >> Op 17 mei 2021, om 16:49 heeft Rafał Miłecki het volgende >> geschreven: >>> >>> From: Rafał Miłecki >>> >>> Interfaces need to be assigned to devices. For that purpose a "device" >>> option should be more accurate than "ifname" one. >>> >>> For backward compatibility add a temporary config translation. >>> >>> Config example: >>> >>> config device >>> option name 'lan' >>> option type 'bridge' >>> list ports 'lan1' >>> list ports 'lan2' >>> >>> config interface 'home' >>> option device 'lan' >>> option proto 'static' >> A bit off-topic: the disparency between config section names and an option >> named name is not always clear. > > What do you mean by "not always"? I think it should be consistent: > "interface" UCI section should always use "device" for referencing > "device" UCI section (by its "name"). Indeed. > As for "name" option in the "device" UCI section I thought we could make > it section name instead, but it can't be done due to UCI limitations for > section names: > > [2021-05-14] [16:59:17 CEST] jow: nbd: quick question - could we > have "config device " and drop "option name " ? i see two > advantages: > [2021-05-14] [16:59:21 CEST] 1. it would not allow duplicated names > [2021-05-14] [16:59:21 CEST] 2. referencing devices from "config > interface" would be more natural > [2021-05-14] [17:06:32 CEST] rmilecki: uci section names have > restrictions on what characters are allowed > [2021-05-14] [17:09:40 CEST] nbd: right, thanks > [2021-05-14] [17:10:15 CEST] ah yes, the babeld uci integration > used to do this (interface name in section name), but we had to drop it Ah, that explains. Your reasoning why using section names would be better stands. Assuming the uci limitation in this case concerns not having dashes in section names: Maybe, as long as uci has these restrictions on section naming and the dash cannot be used therein, one could still use an (allowed) device section name and use that for reference from interface sections and using an device option name to override (the name defaults to the section name), or, change how netifd names devices that it (implicitly) creates. Good luck with your endeavour. That proces itself helps understanding the currently implemented model. --- End Message --- ___ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Re: [PATCH netifd] interface: rename "ifname" attribute to "device"
On 17.05.2021 17:32, Paul Oranje wrote: Op 17 mei 2021, om 16:49 heeft Rafał Miłecki het volgende geschreven: From: Rafał Miłecki Interfaces need to be assigned to devices. For that purpose a "device" option should be more accurate than "ifname" one. For backward compatibility add a temporary config translation. Config example: config device option name 'lan' option type 'bridge' list ports 'lan1' list ports 'lan2' config interface 'home' option device 'lan' option proto 'static' A bit off-topic: the disparency between config section names and an option named name is not always clear. What do you mean by "not always"? I think it should be consistent: "interface" UCI section should always use "device" for referencing "device" UCI section (by its "name"). As for "name" option in the "device" UCI section I thought we could make it section name instead, but it can't be done due to UCI limitations for section names: [2021-05-14] [16:59:17 CEST] jow: nbd: quick question - could we have "config device " and drop "option name " ? i see two advantages: [2021-05-14] [16:59:21 CEST] 1. it would not allow duplicated names [2021-05-14] [16:59:21 CEST] 2. referencing devices from "config interface" would be more natural [2021-05-14] [17:06:32 CEST] rmilecki: uci section names have restrictions on what characters are allowed [2021-05-14] [17:09:40 CEST] nbd: right, thanks [2021-05-14] [17:10:15 CEST] ah yes, the babeld uci integration used to do this (interface name in section name), but we had to drop it ___ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Re: [PATCH netifd] interface: rename "ifname" attribute to "device"
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 ---
Op 17 mei 2021, om 16:49 heeft Rafał Miłecki het volgende
geschreven:
>
> From: Rafał Miłecki
>
> Interfaces need to be assigned to devices. For that purpose a "device"
> option should be more accurate than "ifname" one.
>
> For backward compatibility add a temporary config translation.
>
> Config example:
>
> config device
> option name 'lan'
> option type 'bridge'
> list ports 'lan1'
> list ports 'lan2'
>
> config interface 'home'
> option device 'lan'
> option proto 'static'
A bit off-topic: the disparency between config section names and an option
named name is not always clear.
>
> Signed-off-by: Rafał Miłecki
> ---
> config.c| 21 +
> interface.c | 22 +++---
> interface.h | 2 +-
> 3 files changed, 33 insertions(+), 12 deletions(-)
>
> diff --git a/config.c b/config.c
> index 1f4560f..b461b20 100644
> --- a/config.c
> +++ b/config.c
> @@ -148,6 +148,25 @@ config_parse_bridge_interface(struct uci_section *s,
> struct device_type *devtype
> return 0;
> }
>
> +/**
> + * config_fixup_interface_device - translate deprecated "ifname" option
> + *
> + * Initially "interface" sections were using "ifname" for specifying device.
> + * That has been replaced by the "device" option. For backward compatibility
> + * translate it.
> + */
> +static void config_fixup_interface_device(struct uci_section *s)
> +{
> + const char *ifname;
> +
> + if (uci_lookup_option(uci_ctx, s, "device"))
> + return;
> +
> + ifname = uci_lookup_option_string(uci_ctx, s, "ifname");
> + if (ifname)
> + config_fixup_bridge_var(s, "device", ifname);
> +}
> +
> static void
> config_parse_interface(struct uci_section *s, bool alias)
> {
> @@ -161,6 +180,8 @@ config_parse_interface(struct uci_section *s, bool alias)
> if (disabled && !strcmp(disabled, "1"))
> return;
>
> + config_fixup_interface_device(s);
> +
> blob_buf_init(&b, 0);
>
> if (!alias)
> diff --git a/interface.c b/interface.c
> index ccae915..95f3391 100644
> --- a/interface.c
> +++ b/interface.c
> @@ -30,7 +30,7 @@ struct vlist_tree interfaces;
> static LIST_HEAD(iface_all_users);
>
> enum {
> - IFACE_ATTR_IFNAME,
> + IFACE_ATTR_DEVICE,
> IFACE_ATTR_PROTO,
> IFACE_ATTR_AUTO,
> IFACE_ATTR_JAIL,
> @@ -55,8 +55,8 @@ enum {
> };
>
> static const struct blobmsg_policy iface_attrs[IFACE_ATTR_MAX] = {
> + [IFACE_ATTR_DEVICE] = { .name = "device", .type = BLOBMSG_TYPE_STRING },
> [IFACE_ATTR_PROTO] = { .name = "proto", .type = BLOBMSG_TYPE_STRING },
> - [IFACE_ATTR_IFNAME] = { .name = "ifname", .type = BLOBMSG_TYPE_STRING },
> [IFACE_ATTR_AUTO] = { .name = "auto", .type = BLOBMSG_TYPE_BOOL },
> [IFACE_ATTR_JAIL] = { .name = "jail", .type = BLOBMSG_TYPE_STRING },
> [IFACE_ATTR_JAIL_IFNAME] = { .name = "jail_ifname", .type =
> BLOBMSG_TYPE_STRING },
> @@ -629,9 +629,9 @@ interface_claim_device(struct interface *iface)
> parent = vlist_find(&interfaces, iface->parent_ifname, parent,
> node);
> iface->parent_iface.cb = interface_alias_cb;
> interface_add_user(&iface->parent_iface, parent);
> - } else if (iface->ifname &&
> + } else if (iface->device &&
> !(iface->proto_handler->flags & PROTO_FLAG_NODEV)) {
> - dev = device_get(iface->ifname, true);
> + dev = device_get(iface->device, true);
> interface_set_device_config(iface, dev);
> } else {
> dev = iface->ext_dev.dev;
> @@ -927,8 +927,8 @@ static bool __interface_add(struct interface *iface,
> struct blob_attr *config, b
> if (!iface->parent_ifname)
> return false;
> } else {
> - if ((cur = tb[IFACE_ATTR_IFNAME]))
> - iface->ifname = blobmsg_data(cur);
> + if ((cur = tb[IFACE_ATTR_DEVICE]))
> + iface->device = blobmsg_data(cur);
> }
>
> if (iface->dynamic) {
> @@ -1204,7 +1204,7 @@ interface_start_jail(const char *jail, const pid_t
> netns_pid)
>* list, so we can mess with it :)
>*/
> if (iface->jail_ifname)
> - iface->ifname = iface->jail_ifname;
> + iface->device = iface->jail_ifname;
>
> interface_do_reload(iface);
> interface_set_up(iface);
> @@ -1245,9 +1245,9 @@ interface_stop_jail(const char *jail, const pid_t
> netns_pid)
> if (!iface->jail || strcmp(iface->jail, jail))
> continue;
>
> - orig_ifname = iface->ifname;
> + ori
Re: [PATCH netifd] interface: rename "ifname" attribute to "device"
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 ---
Op 17 mei 2021, om 16:49 heeft Rafał Miłecki het volgende
geschreven:
>
> From: Rafał Miłecki
>
> Interfaces need to be assigned to devices. For that purpose a "device"
> option should be more accurate than "ifname" one.
>
> For backward compatibility add a temporary config translation.
>
> Config example:
>
> config device
> option name 'lan'
> option type 'bridge'
> list ports 'lan1'
> list ports 'lan2'
>
> config interface 'home'
> option device 'lan'
> option proto 'static'
A bit off-topic: the disparency between config section names and an option
named name is not always clear.
>
> Signed-off-by: Rafał Miłecki
> ---
> config.c| 21 +
> interface.c | 22 +++---
> interface.h | 2 +-
> 3 files changed, 33 insertions(+), 12 deletions(-)
>
> diff --git a/config.c b/config.c
> index 1f4560f..b461b20 100644
> --- a/config.c
> +++ b/config.c
> @@ -148,6 +148,25 @@ config_parse_bridge_interface(struct uci_section *s,
> struct device_type *devtype
> return 0;
> }
>
> +/**
> + * config_fixup_interface_device - translate deprecated "ifname" option
> + *
> + * Initially "interface" sections were using "ifname" for specifying device.
> + * That has been replaced by the "device" option. For backward compatibility
> + * translate it.
> + */
> +static void config_fixup_interface_device(struct uci_section *s)
> +{
> + const char *ifname;
> +
> + if (uci_lookup_option(uci_ctx, s, "device"))
> + return;
> +
> + ifname = uci_lookup_option_string(uci_ctx, s, "ifname");
> + if (ifname)
> + config_fixup_bridge_var(s, "device", ifname);
> +}
> +
> static void
> config_parse_interface(struct uci_section *s, bool alias)
> {
> @@ -161,6 +180,8 @@ config_parse_interface(struct uci_section *s, bool alias)
> if (disabled && !strcmp(disabled, "1"))
> return;
>
> + config_fixup_interface_device(s);
> +
> blob_buf_init(&b, 0);
>
> if (!alias)
> diff --git a/interface.c b/interface.c
> index ccae915..95f3391 100644
> --- a/interface.c
> +++ b/interface.c
> @@ -30,7 +30,7 @@ struct vlist_tree interfaces;
> static LIST_HEAD(iface_all_users);
>
> enum {
> - IFACE_ATTR_IFNAME,
> + IFACE_ATTR_DEVICE,
> IFACE_ATTR_PROTO,
> IFACE_ATTR_AUTO,
> IFACE_ATTR_JAIL,
> @@ -55,8 +55,8 @@ enum {
> };
>
> static const struct blobmsg_policy iface_attrs[IFACE_ATTR_MAX] = {
> + [IFACE_ATTR_DEVICE] = { .name = "device", .type = BLOBMSG_TYPE_STRING },
> [IFACE_ATTR_PROTO] = { .name = "proto", .type = BLOBMSG_TYPE_STRING },
> - [IFACE_ATTR_IFNAME] = { .name = "ifname", .type = BLOBMSG_TYPE_STRING },
> [IFACE_ATTR_AUTO] = { .name = "auto", .type = BLOBMSG_TYPE_BOOL },
> [IFACE_ATTR_JAIL] = { .name = "jail", .type = BLOBMSG_TYPE_STRING },
> [IFACE_ATTR_JAIL_IFNAME] = { .name = "jail_ifname", .type =
> BLOBMSG_TYPE_STRING },
> @@ -629,9 +629,9 @@ interface_claim_device(struct interface *iface)
> parent = vlist_find(&interfaces, iface->parent_ifname, parent,
> node);
> iface->parent_iface.cb = interface_alias_cb;
> interface_add_user(&iface->parent_iface, parent);
> - } else if (iface->ifname &&
> + } else if (iface->device &&
> !(iface->proto_handler->flags & PROTO_FLAG_NODEV)) {
> - dev = device_get(iface->ifname, true);
> + dev = device_get(iface->device, true);
> interface_set_device_config(iface, dev);
> } else {
> dev = iface->ext_dev.dev;
> @@ -927,8 +927,8 @@ static bool __interface_add(struct interface *iface,
> struct blob_attr *config, b
> if (!iface->parent_ifname)
> return false;
> } else {
> - if ((cur = tb[IFACE_ATTR_IFNAME]))
> - iface->ifname = blobmsg_data(cur);
> + if ((cur = tb[IFACE_ATTR_DEVICE]))
> + iface->device = blobmsg_data(cur);
> }
>
> if (iface->dynamic) {
> @@ -1204,7 +1204,7 @@ interface_start_jail(const char *jail, const pid_t
> netns_pid)
>* list, so we can mess with it :)
>*/
> if (iface->jail_ifname)
> - iface->ifname = iface->jail_ifname;
> + iface->device = iface->jail_ifname;
>
> interface_do_reload(iface);
> interface_set_up(iface);
> @@ -1245,9 +1245,9 @@ interface_stop_jail(const char *jail, const pid_t
> netns_pid)
> if (!iface->jail || strcmp(iface->jail, jail))
> continue;
>
> - orig_ifname = iface->ifname;
> + ori
[PATCH netifd] interface: rename "ifname" attribute to "device"
From: Rafał Miłecki
Interfaces need to be assigned to devices. For that purpose a "device"
option should be more accurate than "ifname" one.
For backward compatibility add a temporary config translation.
Config example:
config device
option name 'lan'
option type 'bridge'
list ports 'lan1'
list ports 'lan2'
config interface 'home'
option device 'lan'
option proto 'static'
Signed-off-by: Rafał Miłecki
---
config.c| 21 +
interface.c | 22 +++---
interface.h | 2 +-
3 files changed, 33 insertions(+), 12 deletions(-)
diff --git a/config.c b/config.c
index 1f4560f..b461b20 100644
--- a/config.c
+++ b/config.c
@@ -148,6 +148,25 @@ config_parse_bridge_interface(struct uci_section *s,
struct device_type *devtype
return 0;
}
+/**
+ * config_fixup_interface_device - translate deprecated "ifname" option
+ *
+ * Initially "interface" sections were using "ifname" for specifying device.
+ * That has been replaced by the "device" option. For backward compatibility
+ * translate it.
+ */
+static void config_fixup_interface_device(struct uci_section *s)
+{
+ const char *ifname;
+
+ if (uci_lookup_option(uci_ctx, s, "device"))
+ return;
+
+ ifname = uci_lookup_option_string(uci_ctx, s, "ifname");
+ if (ifname)
+ config_fixup_bridge_var(s, "device", ifname);
+}
+
static void
config_parse_interface(struct uci_section *s, bool alias)
{
@@ -161,6 +180,8 @@ config_parse_interface(struct uci_section *s, bool alias)
if (disabled && !strcmp(disabled, "1"))
return;
+ config_fixup_interface_device(s);
+
blob_buf_init(&b, 0);
if (!alias)
diff --git a/interface.c b/interface.c
index ccae915..95f3391 100644
--- a/interface.c
+++ b/interface.c
@@ -30,7 +30,7 @@ struct vlist_tree interfaces;
static LIST_HEAD(iface_all_users);
enum {
- IFACE_ATTR_IFNAME,
+ IFACE_ATTR_DEVICE,
IFACE_ATTR_PROTO,
IFACE_ATTR_AUTO,
IFACE_ATTR_JAIL,
@@ -55,8 +55,8 @@ enum {
};
static const struct blobmsg_policy iface_attrs[IFACE_ATTR_MAX] = {
+ [IFACE_ATTR_DEVICE] = { .name = "device", .type = BLOBMSG_TYPE_STRING },
[IFACE_ATTR_PROTO] = { .name = "proto", .type = BLOBMSG_TYPE_STRING },
- [IFACE_ATTR_IFNAME] = { .name = "ifname", .type = BLOBMSG_TYPE_STRING },
[IFACE_ATTR_AUTO] = { .name = "auto", .type = BLOBMSG_TYPE_BOOL },
[IFACE_ATTR_JAIL] = { .name = "jail", .type = BLOBMSG_TYPE_STRING },
[IFACE_ATTR_JAIL_IFNAME] = { .name = "jail_ifname", .type =
BLOBMSG_TYPE_STRING },
@@ -629,9 +629,9 @@ interface_claim_device(struct interface *iface)
parent = vlist_find(&interfaces, iface->parent_ifname, parent,
node);
iface->parent_iface.cb = interface_alias_cb;
interface_add_user(&iface->parent_iface, parent);
- } else if (iface->ifname &&
+ } else if (iface->device &&
!(iface->proto_handler->flags & PROTO_FLAG_NODEV)) {
- dev = device_get(iface->ifname, true);
+ dev = device_get(iface->device, true);
interface_set_device_config(iface, dev);
} else {
dev = iface->ext_dev.dev;
@@ -927,8 +927,8 @@ static bool __interface_add(struct interface *iface, struct
blob_attr *config, b
if (!iface->parent_ifname)
return false;
} else {
- if ((cur = tb[IFACE_ATTR_IFNAME]))
- iface->ifname = blobmsg_data(cur);
+ if ((cur = tb[IFACE_ATTR_DEVICE]))
+ iface->device = blobmsg_data(cur);
}
if (iface->dynamic) {
@@ -1204,7 +1204,7 @@ interface_start_jail(const char *jail, const pid_t
netns_pid)
* list, so we can mess with it :)
*/
if (iface->jail_ifname)
- iface->ifname = iface->jail_ifname;
+ iface->device = iface->jail_ifname;
interface_do_reload(iface);
interface_set_up(iface);
@@ -1245,9 +1245,9 @@ interface_stop_jail(const char *jail, const pid_t
netns_pid)
if (!iface->jail || strcmp(iface->jail, jail))
continue;
- orig_ifname = iface->ifname;
+ orig_ifname = iface->device;
if (iface->jail_ifname)
- iface->ifname = iface->jail_ifname;
+ iface->device = iface->jail_ifname;
interface_do_reload(iface);
interface_set_down(iface);
@@ -1340,7 +1340,7 @@ interface_change_config(struct interface *if_old, struct
interface *if_new)
if (!reload && interface_device_config_changed(if_old, if_new))
reload = true;
- if (FIELD_CHANGED_STR(ifname) ||
+ if (FIELD_CHANGED_STR(device) ||
