diff mbox series

[netifd] config: support bridge designed UCI section

Message ID 20210511171407.17078-1-zajec5@gmail.com
State Changes Requested
Delegated to: Rafał Miłecki
Headers show
Series [netifd] config: support bridge designed UCI section | expand

Commit Message

Rafał Miłecki May 11, 2021, 5:14 p.m. UTC
From: Rafał Miłecki <rafal@milecki.pl>

Network layer 2 devices should have their own UCI section types. They
differ so much that each device type requires a custom handling anyway.
Currently there is "type" option used to distinguish them while UCI
supports different section types right for that purpose. This change
will result in cleaner UCI and UI code.

Example UCI section:

config bridge
	option name 'foo'
	list ports 'lan1'
	list ports 'lan2'

While introducing this new bridge section a new option was added for
storing bridge port names: "ports". It's clearer than previously used
"ifname". A simple validation code is present to make sure "ports" is
used and contains a list of ports.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 bridge.c | 14 +++++++++-----
 config.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 61 insertions(+), 5 deletions(-)

Comments

Hans Dedecker May 12, 2021, 6:37 p.m. UTC | #1
On Tue, May 11, 2021 at 7:14 PM Rafał Miłecki <zajec5@gmail.com> wrote:
>
> From: Rafał Miłecki <rafal@milecki.pl>
>
> Network layer 2 devices should have their own UCI section types. They
> differ so much that each device type requires a custom handling anyway.
> Currently there is "type" option used to distinguish them while UCI
> supports different section types right for that purpose. This change
> will result in cleaner UCI and UI code.
>
> Example UCI section:
>
> config bridge
>         option name 'foo'
>         list ports 'lan1'
>         list ports 'lan2'
>
> While introducing this new bridge section a new option was added for
> storing bridge port names: "ports". It's clearer than previously used
> "ifname". A simple validation code is present to make sure "ports" is
> used and contains a list of ports.
>
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
Acked-by: Hans Dedecker <dedeckeh@gmail.com>
> ---
>  bridge.c | 14 +++++++++-----
>  config.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 61 insertions(+), 5 deletions(-)
>
> diff --git a/bridge.c b/bridge.c
> index 099dfe4..ce49a74 100644
> --- a/bridge.c
> +++ b/bridge.c
> @@ -23,7 +23,8 @@
>  #include "system.h"
>
>  enum {
> -       BRIDGE_ATTR_IFNAME,
> +       BRIDGE_ATTR_PORTS,
> +       BRIDGE_ATTR_IFNAME, /* Deprecated */
>         BRIDGE_ATTR_STP,
>         BRIDGE_ATTR_FORWARD_DELAY,
>         BRIDGE_ATTR_PRIORITY,
> @@ -44,6 +45,7 @@ enum {
>  };
>
>  static const struct blobmsg_policy bridge_attrs[__BRIDGE_ATTR_MAX] = {
> +       [BRIDGE_ATTR_PORTS] = { "ports", BLOBMSG_TYPE_ARRAY },
>         [BRIDGE_ATTR_IFNAME] = { "ifname", BLOBMSG_TYPE_ARRAY },
>         [BRIDGE_ATTR_STP] = { "stp", BLOBMSG_TYPE_BOOL },
>         [BRIDGE_ATTR_FORWARD_DELAY] = { "forward_delay", BLOBMSG_TYPE_INT32 },
> @@ -104,7 +106,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 +855,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 +972,9 @@ 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];
> +       if (!bst->ports)
> +               bst->ports = tb_br[BRIDGE_ATTR_IFNAME];
>         device_init_settings(dev, tb_dev);
>         bridge_apply_settings(bst, tb_br);
>
> diff --git a/config.c b/config.c
> index fa7cbe4..4cc5a61 100644
> --- a/config.c
> +++ b/config.c
> @@ -223,6 +223,57 @@ config_parse_rule(struct uci_section *s, bool v6)
>         iprule_add(blob_data(b.head), v6);
>  }
>
> +/**
> + * config_init_bridges - create bridges for new syntax UCI sections
> + *
> + * The new syntax uses dedicated UCI "bridge" sections for describing bridges.
> + * They use "ports" list instead of "ifname" for specifying bridge ports.
> + */
> +static void config_init_bridges()
> +{
> +       struct uci_element *e;
> +
> +       uci_foreach_element(&uci_network->sections, e) {
> +               struct uci_section *s = uci_to_section(e);
> +               struct device_type *devtype;
> +               struct uci_option *o;
> +               struct device *dev;
> +               const char *name;
> +
> +               if (strcmp(s->type, "bridge"))
> +                       continue;
> +
> +               name = uci_lookup_option_string(uci_ctx, s, "name");
> +               if (!name)
> +                       continue;
> +
> +               devtype = device_type_get("bridge");
> +               if (!devtype)
> +                       continue;
> +
> +               config_fixup_bridge_vlan_filtering(s, name);
> +               o = uci_lookup_option(uci_ctx, s, "ifname");
> +               if (o) {
> +                       netifd_log_message(L_WARNING, "Unsupported \"ifname\" bridge option\n");
> +                       continue;
> +               }
> +               o = uci_lookup_option(uci_ctx, s, "ports");
> +               if (o && o->type != UCI_TYPE_LIST) {
> +                       netifd_log_message(L_WARNING, "Invalid \"ports\" option format\n");
> +                       continue;
> +               }
> +
> +               blob_buf_init(&b, 0);
> +               uci_to_blob(&b, s, devtype->config_params);
> +
> +               dev = device_create(name, devtype, b.head);
> +               if (!dev)
> +                       continue;
> +
> +               dev->default_config = false;
> +       }
> +}
> +
>  static void
>  config_init_devices(bool bridge)
>  {
> @@ -737,6 +788,7 @@ config_init_all(void)
>         device_lock();
>
>         device_reset_config();
> +       config_init_bridges();
>         config_init_devices(true);
>         config_init_vlans();
>         config_init_devices(false);
> --
> 2.26.2
>
diff mbox series

Patch

diff --git a/bridge.c b/bridge.c
index 099dfe4..ce49a74 100644
--- a/bridge.c
+++ b/bridge.c
@@ -23,7 +23,8 @@ 
 #include "system.h"
 
 enum {
-	BRIDGE_ATTR_IFNAME,
+	BRIDGE_ATTR_PORTS,
+	BRIDGE_ATTR_IFNAME, /* Deprecated */
 	BRIDGE_ATTR_STP,
 	BRIDGE_ATTR_FORWARD_DELAY,
 	BRIDGE_ATTR_PRIORITY,
@@ -44,6 +45,7 @@  enum {
 };
 
 static const struct blobmsg_policy bridge_attrs[__BRIDGE_ATTR_MAX] = {
+	[BRIDGE_ATTR_PORTS] = { "ports", BLOBMSG_TYPE_ARRAY },
 	[BRIDGE_ATTR_IFNAME] = { "ifname", BLOBMSG_TYPE_ARRAY },
 	[BRIDGE_ATTR_STP] = { "stp", BLOBMSG_TYPE_BOOL },
 	[BRIDGE_ATTR_FORWARD_DELAY] = { "forward_delay", BLOBMSG_TYPE_INT32 },
@@ -104,7 +106,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 +855,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 +972,9 @@  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];
+	if (!bst->ports)
+		bst->ports = tb_br[BRIDGE_ATTR_IFNAME];
 	device_init_settings(dev, tb_dev);
 	bridge_apply_settings(bst, tb_br);
 
diff --git a/config.c b/config.c
index fa7cbe4..4cc5a61 100644
--- a/config.c
+++ b/config.c
@@ -223,6 +223,57 @@  config_parse_rule(struct uci_section *s, bool v6)
 	iprule_add(blob_data(b.head), v6);
 }
 
+/**
+ * config_init_bridges - create bridges for new syntax UCI sections
+ *
+ * The new syntax uses dedicated UCI "bridge" sections for describing bridges.
+ * They use "ports" list instead of "ifname" for specifying bridge ports.
+ */
+static void config_init_bridges()
+{
+	struct uci_element *e;
+
+	uci_foreach_element(&uci_network->sections, e) {
+		struct uci_section *s = uci_to_section(e);
+		struct device_type *devtype;
+		struct uci_option *o;
+		struct device *dev;
+		const char *name;
+
+		if (strcmp(s->type, "bridge"))
+			continue;
+
+		name = uci_lookup_option_string(uci_ctx, s, "name");
+		if (!name)
+			continue;
+
+		devtype = device_type_get("bridge");
+		if (!devtype)
+			continue;
+
+		config_fixup_bridge_vlan_filtering(s, name);
+		o = uci_lookup_option(uci_ctx, s, "ifname");
+		if (o) {
+			netifd_log_message(L_WARNING, "Unsupported \"ifname\" bridge option\n");
+			continue;
+		}
+		o = uci_lookup_option(uci_ctx, s, "ports");
+		if (o && o->type != UCI_TYPE_LIST) {
+			netifd_log_message(L_WARNING, "Invalid \"ports\" option format\n");
+			continue;
+		}
+
+		blob_buf_init(&b, 0);
+		uci_to_blob(&b, s, devtype->config_params);
+
+		dev = device_create(name, devtype, b.head);
+		if (!dev)
+			continue;
+
+		dev->default_config = false;
+	}
+}
+
 static void
 config_init_devices(bool bridge)
 {
@@ -737,6 +788,7 @@  config_init_all(void)
 	device_lock();
 
 	device_reset_config();
+	config_init_bridges();
 	config_init_devices(true);
 	config_init_vlans();
 	config_init_devices(false);