Merge pull request #12964 from yuwata/network-bridge-vlan-issue-12958

network: fix BridgeVLAN issue

6 files changed, 74 insertions, 76 deletions

diff --git a/src/network/networkd-brvlan.c b/src/network/networkd-brvlan.c
index 8f9103f146..c3c5d535ac 100644
--- a/src/network/networkd-brvlan.c
+++ b/src/network/networkd-brvlan.c
@@ -19,12 +19,12 @@
 
 static bool is_bit_set(unsigned bit, uint32_t scope) {
         assert(bit < sizeof(scope)*8);
-        return scope & (1 << bit);
+        return scope & (UINT32_C(1) << bit);
 }
 
 static void set_bit(unsigned nr, uint32_t *addr) {
         if (nr < BRIDGE_VLAN_BITMAP_MAX)
-                addr[nr / 32] |= (((uint32_t) 1) << (nr % 32));
+                addr[nr / 32] |= (UINT32_C(1) << (nr % 32));
 }
 
 static int find_next_bit(int i, uint32_t x) {
@@ -44,16 +44,16 @@ static int find_next_bit(int i, uint32_t x) {
 
 static int append_vlan_info_data(Link *const link, sd_netlink_message *req, uint16_t pvid, const uint32_t *br_vid_bitmap, const uint32_t *br_untagged_bitmap) {
         struct bridge_vlan_info br_vlan;
-        int i, j, k, r, done, cnt;
+        int i, j, k, r, cnt;
         uint16_t begin, end;
-        bool untagged = false;
+        bool done, untagged = false;
 
         assert(link);
         assert(req);
         assert(br_vid_bitmap);
         assert(br_untagged_bitmap);
 
-        i = cnt = -1;
+        cnt = 0;
 
         begin = end = UINT16_MAX;
         for (k = 0; k < BRIDGE_VLAN_BITMAP_LEN; k++) {
@@ -63,7 +63,7 @@ static int append_vlan_info_data(Link *const link, sd_netlink_message *req, uint
 
                 base_bit = k * 32;
                 i = -1;
-                done = 0;
+                done = false;
                 do {
                         j = find_next_bit(i, vid_map);
                         if (j > 0) {
@@ -80,7 +80,7 @@ static int append_vlan_info_data(Link *const link, sd_netlink_message *req, uint
                                         goto next;
                                 }
                         } else
-                                done = 1;
+                                done = true;
 
                         if (begin != UINT16_MAX) {
                                 cnt++;
@@ -129,9 +129,8 @@ static int append_vlan_info_data(Link *const link, sd_netlink_message *req, uint
                         i = j;
                 } while (!done);
         }
-        if (!cnt)
-                return -EINVAL;
 
+        assert(cnt > 0);
         return cnt;
 }
 
@@ -149,9 +148,9 @@ static int set_brvlan_handler(sd_netlink *rtnl, sd_netlink_message *m, Link *lin
 
 int br_vlan_configure(Link *link, uint16_t pvid, uint32_t *br_vid_bitmap, uint32_t *br_untagged_bitmap) {
         _cleanup_(sd_netlink_message_unrefp) sd_netlink_message *req = NULL;
-        int r;
-        uint16_t flags;
         sd_netlink *rtnl;
+        uint16_t flags;
+        int r;
 
         assert(link);
         assert(link->manager);
@@ -204,57 +203,15 @@ int br_vlan_configure(Link *link, uint16_t pvid, uint32_t *br_vid_bitmap, uint32
         return 0;
 }
 
-static int parse_vid_range(const char *rvalue, uint16_t *vid, uint16_t *vid_end) {
-        int r;
-        char *p;
-        char *_rvalue = NULL;
-        uint16_t _vid = UINT16_MAX;
-        uint16_t _vid_end = UINT16_MAX;
-
-        assert(rvalue);
-        assert(vid);
-        assert(vid_end);
-
-        _rvalue = strdupa(rvalue);
-        p = strchr(_rvalue, '-');
-        if (p) {
-                *p = '\0';
-                p++;
-                r = parse_vlanid(_rvalue, &_vid);
-                if (r < 0)
-                        return r;
-
-                if (_vid == 0)
-                        return -ERANGE;
-
-                r = parse_vlanid(p, &_vid_end);
-                if (r < 0)
-                        return r;
-
-                if (_vid_end == 0)
-                        return -ERANGE;
-        } else {
-                r = parse_vlanid(_rvalue, &_vid);
-                if (r < 0)
-                        return r;
-
-                if (_vid == 0)
-                        return -ERANGE;
-        }
-
-        *vid = _vid;
-        *vid_end = _vid_end;
-        return r;
-}
-
 int config_parse_brvlan_pvid(const char *unit, const char *filename,
                              unsigned line, const char *section,
                              unsigned section_line, const char *lvalue,
                              int ltype, const char *rvalue, void *data,
                              void *userdata) {
         Network *network = userdata;
-        int r;
         uint16_t pvid;
+        int r;
+
         r = parse_vlanid(rvalue, &pvid);
         if (r < 0)
                 return r;
@@ -271,8 +228,8 @@ int config_parse_brvlan_vlan(const char *unit, const char *filename,
                              int ltype, const char *rvalue, void *data,
                              void *userdata) {
         Network *network = userdata;
-        int r;
         uint16_t vid, vid_end;
+        int r;
 
         assert(filename);
         assert(section);
@@ -286,16 +243,9 @@ int config_parse_brvlan_vlan(const char *unit, const char *filename,
                 return 0;
         }
 
-        if (UINT16_MAX == vid_end)
-                set_bit(vid++, network->br_vid_bitmap);
-        else {
-                if (vid >= vid_end) {
-                        log_syntax(unit, LOG_ERR, filename, line, 0, "Invalid VLAN range, ignoring %s", rvalue);
-                        return 0;
-                }
-                for (; vid <= vid_end; vid++)
-                        set_bit(vid, network->br_vid_bitmap);
-        }
+        for (; vid <= vid_end; vid++)
+                set_bit(vid, network->br_vid_bitmap);
+
         network->use_br_vlan = true;
         return 0;
 }
@@ -321,19 +271,11 @@ int config_parse_brvlan_untagged(const char *unit, const char *filename,
                 return 0;
         }
 
-        if (UINT16_MAX == vid_end) {
+        for (; vid <= vid_end; vid++) {
                 set_bit(vid, network->br_vid_bitmap);
                 set_bit(vid, network->br_untagged_bitmap);
-        } else {
-                if (vid >= vid_end) {
-                        log_syntax(unit, LOG_ERR, filename, line, 0, "Invalid VLAN range, ignoring %s", rvalue);
-                        return 0;
-                }
-                for (; vid <= vid_end; vid++) {
-                        set_bit(vid, network->br_vid_bitmap);
-                        set_bit(vid, network->br_untagged_bitmap);
-                }
         }
+
         network->use_br_vlan = true;
         return 0;
 }
diff --git a/src/shared/vlan-util.c b/src/shared/vlan-util.c
index 2f9df7dd1b..a4b42df85b 100644
--- a/src/shared/vlan-util.c
+++ b/src/shared/vlan-util.c
@@ -22,6 +22,22 @@ int parse_vlanid(const char *p, uint16_t *ret) {
         return 0;
 }
 
+int parse_vid_range(const char *p, uint16_t *vid, uint16_t *vid_end) {
+        unsigned lower, upper;
+        int r;
+
+        r = parse_range(p, &lower, &upper);
+        if (r < 0)
+                return r;
+
+        if (lower > VLANID_MAX || upper > VLANID_MAX || lower > upper)
+                return -EINVAL;
+
+        *vid = lower;
+        *vid_end = upper;
+        return 0;
+}
+
 int config_parse_default_port_vlanid(
                 const char *unit,
                 const char *filename,
diff --git a/src/shared/vlan-util.h b/src/shared/vlan-util.h
index ebe4331ed4..c55adee3c0 100644
--- a/src/shared/vlan-util.h
+++ b/src/shared/vlan-util.h
@@ -15,6 +15,7 @@ static inline bool vlanid_is_valid(uint16_t id) {
 }
 
 int parse_vlanid(const char *p, uint16_t *ret);
+int parse_vid_range(const char *p, uint16_t *vid, uint16_t *vid_end);
 
 CONFIG_PARSER_PROTOTYPE(config_parse_default_port_vlanid);
 CONFIG_PARSER_PROTOTYPE(config_parse_vlanid);
diff --git a/test/test-network/conf/26-bridge-vlan-master.network b/test/test-network/conf/26-bridge-vlan-master.network
new file mode 100644
index 0000000000..8493e32525
--- /dev/null
+++ b/test/test-network/conf/26-bridge-vlan-master.network
@@ -0,0 +1,8 @@
+[Match]
+Name=bridge99
+
+[Network]
+IPv6AcceptRA=false
+
+[BridgeVLAN]
+VLAN=4060-4094
diff --git a/test/test-network/conf/26-bridge-vlan-slave.network b/test/test-network/conf/26-bridge-vlan-slave.network
new file mode 100644
index 0000000000..ba50508afb
--- /dev/null
+++ b/test/test-network/conf/26-bridge-vlan-slave.network
@@ -0,0 +1,9 @@
+[Match]
+Name=test1
+
+[Network]
+IPv6AcceptRA=no
+Bridge=bridge99
+
+[BridgeVLAN]
+VLAN=4064-4094
diff --git a/test/test-network/systemd-networkd-tests.py b/test/test-network/systemd-networkd-tests.py
index 849ea97b48..ba8fb7255f 100755
--- a/test/test-network/systemd-networkd-tests.py
+++ b/test/test-network/systemd-networkd-tests.py
@@ -1861,6 +1861,8 @@ class NetworkdBridgeTests(unittest.TestCase, Utilities):
         '26-bridge.netdev',
         '26-bridge-slave-interface-1.network',
         '26-bridge-slave-interface-2.network',
+        '26-bridge-vlan-master.network',
+        '26-bridge-vlan-slave.network',
         'bridge99-ignore-carrier-loss.network',
         'bridge99.network']
 
@@ -1877,6 +1879,26 @@ class NetworkdBridgeTests(unittest.TestCase, Utilities):
         remove_unit_from_networkd_path(self.units)
         stop_networkd(show_logs=True)
 
+    def test_bridge_vlan(self):
+        copy_unit_to_networkd_unit_path('11-dummy.netdev', '26-bridge-vlan-slave.network',
+                                        '26-bridge.netdev', '26-bridge-vlan-master.network')
+        start_networkd()
+        wait_online(['test1:enslaved', 'bridge99:degraded'])
+
+        output = check_output('bridge vlan show dev test1')
+        print(output)
+        self.assertNotRegex(output, '4063')
+        for i in range(4064, 4095):
+            self.assertRegex(output, f'{i}')
+        self.assertNotRegex(output, '4095')
+
+        output = check_output('bridge vlan show dev bridge99')
+        print(output)
+        self.assertNotRegex(output, '4059')
+        for i in range(4060, 4095):
+            self.assertRegex(output, f'{i}')
+        self.assertNotRegex(output, '4095')
+
     def test_bridge_property(self):
         copy_unit_to_networkd_unit_path('11-dummy.netdev', '12-dummy.netdev', '26-bridge.netdev',
                                         '26-bridge-slave-interface-1.network', '26-bridge-slave-interface-2.network',