| Message ID | 20210517144925.4913-1-zajec5@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Rafał Miłecki |
| Headers | show |
| Series | [netifd] interface: rename "ifname" attribute to "device" | expand |
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. Op 17 mei 2021, om 16:49 heeft Rafał Miłecki <zajec5@gmail.com> het volgende geschreven: > > From: Rafał Miłecki <rafal@milecki.pl> > > 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 <rafal@milecki.pl> > --- > 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) || > if_old->proto_handler != if_new->proto_handler) > reload = true; > > @@ -1379,7 +1379,7 @@ interface_change_config(struct interface *if_old, struct interface *if_new) > > if_old->jail_ifname = if_new->jail_ifname; > > - if_old->ifname = if_new->ifname; > + if_old->device = if_new->device; > if_old->parent_ifname = if_new->parent_ifname; > if_old->dynamic = if_new->dynamic; > if_old->proto_handler = if_new->proto_handler; > diff --git a/interface.h b/interface.h > index 9c136b6..1767bf1 100644 > --- a/interface.h > +++ b/interface.h > @@ -107,7 +107,7 @@ struct interface { > enum interface_event hotplug_ev; > > const char *name; > - const char *ifname; > + const char *device; > char *jail; > char *jail_ifname; > int netns_fd; > -- > 2.26.2 > > > _______________________________________________ > openwrt-devel mailing list > openwrt-devel@lists.openwrt.org > https://lists.openwrt.org/mailman/listinfo/openwrt-devel
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. Op 17 mei 2021, om 16:49 heeft Rafał Miłecki <zajec5@gmail.com> het volgende geschreven: > > From: Rafał Miłecki <rafal@milecki.pl> > > 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 <rafal@milecki.pl> > --- > 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) || > if_old->proto_handler != if_new->proto_handler) > reload = true; > > @@ -1379,7 +1379,7 @@ interface_change_config(struct interface *if_old, struct interface *if_new) > > if_old->jail_ifname = if_new->jail_ifname; > > - if_old->ifname = if_new->ifname; > + if_old->device = if_new->device; > if_old->parent_ifname = if_new->parent_ifname; > if_old->dynamic = if_new->dynamic; > if_old->proto_handler = if_new->proto_handler; > diff --git a/interface.h b/interface.h > index 9c136b6..1767bf1 100644 > --- a/interface.h > +++ b/interface.h > @@ -107,7 +107,7 @@ struct interface { > enum interface_event hotplug_ev; > > const char *name; > - const char *ifname; > + const char *device; > char *jail; > char *jail_ifname; > int netns_fd; > -- > 2.26.2 > > > _______________________________________________ > openwrt-devel mailing list > openwrt-devel@lists.openwrt.org > https://lists.openwrt.org/mailman/listinfo/openwrt-devel
On 17.05.2021 17:32, Paul Oranje wrote: > Op 17 mei 2021, om 16:49 heeft Rafał Miłecki <zajec5@gmail.com> het volgende geschreven: >> >> From: Rafał Miłecki <rafal@milecki.pl> >> >> 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] <rmilecki> jow: nbd: quick question - could we have "config device <foo>" and drop "option name <foo>" ? i see two advantages: [2021-05-14] [16:59:21 CEST] <rmilecki> 1. it would not allow duplicated names [2021-05-14] [16:59:21 CEST] <rmilecki> 2. referencing devices from "config interface" would be more natural [2021-05-14] [17:06:32 CEST] <nbd> rmilecki: uci section names have restrictions on what characters are allowed [2021-05-14] [17:09:40 CEST] <rmilecki> nbd: right, thanks [2021-05-14] [17:10:15 CEST] <zorun> ah yes, the babeld uci integration used to do this (interface name in section name), but we had to drop it
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. > Op 17 mei 2021, om 17:47 heeft Rafał Miłecki <zajec5@gmail.com> het volgende geschreven: > > On 17.05.2021 17:32, Paul Oranje wrote: >> Op 17 mei 2021, om 16:49 heeft Rafał Miłecki <zajec5@gmail.com> het volgende geschreven: >>> >>> From: Rafał Miłecki <rafal@milecki.pl> >>> >>> 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] <rmilecki> jow: nbd: quick question - could we have "config device <foo>" and drop "option name <foo>" ? i see two advantages: > [2021-05-14] [16:59:21 CEST] <rmilecki> 1. it would not allow duplicated names > [2021-05-14] [16:59:21 CEST] <rmilecki> 2. referencing devices from "config interface" would be more natural > [2021-05-14] [17:06:32 CEST] <nbd> rmilecki: uci section names have restrictions on what characters are allowed > [2021-05-14] [17:09:40 CEST] <rmilecki> nbd: right, thanks > [2021-05-14] [17:10:15 CEST] <zorun> 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.
On 2021-05-17 16:49, Rafał Miłecki wrote: > From: Rafał Miłecki <rafal@milecki.pl> > > 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 <rafal@milecki.pl> 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
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) || if_old->proto_handler != if_new->proto_handler) reload = true; @@ -1379,7 +1379,7 @@ interface_change_config(struct interface *if_old, struct interface *if_new) if_old->jail_ifname = if_new->jail_ifname; - if_old->ifname = if_new->ifname; + if_old->device = if_new->device; if_old->parent_ifname = if_new->parent_ifname; if_old->dynamic = if_new->dynamic; if_old->proto_handler = if_new->proto_handler; diff --git a/interface.h b/interface.h index 9c136b6..1767bf1 100644 --- a/interface.h +++ b/interface.h @@ -107,7 +107,7 @@ struct interface { enum interface_event hotplug_ev; const char *name; - const char *ifname; + const char *device; char *jail; char *jail_ifname; int netns_fd;