Re: [PATCH V2 netifd] interface: rename "ifname" attribute to "device"

2021-05-25 Thread Felix Fietkau

On 2021-05-24 15:35, 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 'br-lan'
>   option type 'bridge'
>   list ports 'lan1'
>   list ports 'lan2'
> 
> config interface 'lan'
>   option device 'br-lan'
>   option proto 'static'
> 
> Signed-off-by: Rafał Miłecki 
I think the code for backwards compatibility would be a lot simpler if
you add both IFACE_ATTR_DEVICE and IFACE_ATTR_IFNAME and initialize
tb[IFACE_ATTR_DEVICE] to tb[IFACE_ATTR_IFNAME] if it's NULL.

- Felix

___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel


[PATCH V2 netifd] interface: rename "ifname" attribute to "device"

2021-05-24 Thread Rafał Miłecki
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 'br-lan'
option type 'bridge'
list ports 'lan1'
list ports 'lan2'

config interface 'lan'
option device 'br-lan'
option proto 'static'

Signed-off-by: Rafał Miłecki 
---
V2: Add translation in the "add_dynamic" ubus method handler
---
 config.c| 21 +
 interface.c | 22 +++---
 interface.h |  2 +-
 ubus.c  | 14 ++
 4 files changed, 47 insertions(+), 12 deletions(-)

diff --git a/config.c b/config.c
index d83ea9c..f6a1071 100644
--- a/config.c
+++ b/config.c
@@ -155,6 +155,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)
 {
@@ -168,6 +187,8 @@ config_parse_interface(struct uci_section *s, bool alias)
if (disabled && !strcmp(disabled, "1"))
return;
 
+   config_fixup_interface_device(s);
+
blob_buf_init(, 0);
 
if (!alias)
diff --git a/interface.c b/interface.c
index a91246a..c298355 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 },
@@ -641,9 +641,9 @@ interface_claim_device(struct interface *iface)
parent = vlist_find(, iface->parent_ifname, parent, 
node);
iface->parent_iface.cb = interface_alias_cb;
interface_add_user(>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;
@@ -939,8 +939,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) {
@@ -1216,7 +1216,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);
@@ -1257,9 +1257,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);
@@ -1352,7 +1352,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