From a358da096f7b2d195dc3b526a7d21efa9ade55b9 Mon Sep 17 00:00:00 2001
From: Thomas Haller <thaller@redhat.com>
Date: Wed, 17 Apr 2019 15:40:32 +0200
Subject: [PATCH] libnm: minor refactoring of
 _nm_utils_bridge_vlan_verify_list()

- if there is only one vlan in the list, then we can return success
  early. That is, because one NMBridgeVlan instance is always valid
  due to the way how users must use the API to construct the element.

- the implementation for check_normalizable is only correct, if there
  are no duplicate or overlapping ranges. Assert for that. In fact,
  all callers first check for errors and then for normalizable errors.

- avoid duplicate calls to nm_bridge_vlan_get_vid_range(). There are
  duplicate assertions that we don't need.

- only check for pvid once per range.

- combine calls to g_hash_table_contains() and g_hash_table_add().
---
 libnm-core/nm-utils.c | 48 ++++++++++++++++++++++++-------------------
 1 file changed, 27 insertions(+), 21 deletions(-)

diff --git a/libnm-core/nm-utils.c b/libnm-core/nm-utils.c
index ea333dc61c..28bc24b1e1 100644
--- a/libnm-core/nm-utils.c
+++ b/libnm-core/nm-utils.c
@@ -6883,28 +6883,33 @@ _nm_utils_bridge_vlan_verify_list (GPtrArray *vlans,
 	gs_unref_hashtable GHashTable *h = NULL;
 	gboolean pvid_found = FALSE;
 
-	if (!vlans || !vlans->len)
+	if (   !vlans
+	    || vlans->len <= 1)
 		return TRUE;
 
 	if (check_normalizable) {
+		guint16 vid_prev_end, vid_start, vid_end;
+
+		nm_assert (_nm_utils_bridge_vlan_verify_list (vlans, FALSE, NULL, setting, property));
+
+		nm_bridge_vlan_get_vid_range (vlans->pdata[0], NULL, &vid_prev_end);
 		for (i = 1; i < vlans->len; i++) {
-			NMBridgeVlan *vlan_prev = vlans->pdata[i - 1];
-			NMBridgeVlan *vlan = vlans->pdata[i];
-			guint16 vid_prev, vid;
+			const NMBridgeVlan *vlan = vlans->pdata[i];
 
-			nm_bridge_vlan_get_vid_range (vlan_prev, &vid_prev, NULL);
-			nm_bridge_vlan_get_vid_range (vlan, &vid, NULL);
+			nm_bridge_vlan_get_vid_range (vlan, &vid_start, &vid_end);
 
-			if (vid_prev > vid) {
+			if (vid_prev_end > vid_start) {
 				g_set_error (error,
 				             NM_CONNECTION_ERROR,
 				             NM_CONNECTION_ERROR_INVALID_PROPERTY,
 				             _("Bridge VLANs %d and %d are not sorted by ascending vid"),
-				             vid_prev,
-				             vid);
+				             vid_prev_end,
+				             vid_start);
 				g_prefix_error (error, "%s.%s: ", setting, property);
 				return FALSE;
 			}
+
+			vid_prev_end = vid_end;
 		}
 		return TRUE;
 	}
@@ -6915,8 +6920,9 @@ _nm_utils_bridge_vlan_verify_list (GPtrArray *vlans,
 		guint16 v, vid_start, vid_end;
 
 		nm_bridge_vlan_get_vid_range (vlan, &vid_start, &vid_end);
+
 		for (v = vid_start; v <= vid_end; v++) {
-			if (g_hash_table_contains (h, GUINT_TO_POINTER (v))) {
+			if (!nm_g_hash_table_add (h, GUINT_TO_POINTER (v))) {
 				g_set_error (error,
 				             NM_CONNECTION_ERROR,
 				             NM_CONNECTION_ERROR_INVALID_PROPERTY,
@@ -6924,19 +6930,19 @@ _nm_utils_bridge_vlan_verify_list (GPtrArray *vlans,
 				g_prefix_error (error, "%s.%s: ", setting, property);
 				return FALSE;
 			}
+		}
 
-			if (nm_bridge_vlan_is_pvid (vlan)) {
-				if (pvid_found) {
-					g_set_error_literal (error,
-					                     NM_CONNECTION_ERROR,
-					                     NM_CONNECTION_ERROR_INVALID_PROPERTY,
-					                     _("only one VLAN can be the PVID"));
-					g_prefix_error (error, "%s.%s: ", setting, property);
-					return FALSE;
-				}
-				pvid_found = TRUE;
+		if (nm_bridge_vlan_is_pvid (vlan)) {
+			if (   vid_start != vid_end
+			    || pvid_found) {
+				g_set_error_literal (error,
+				                     NM_CONNECTION_ERROR,
+				                     NM_CONNECTION_ERROR_INVALID_PROPERTY,
+				                     _("only one VLAN can be the PVID"));
+				g_prefix_error (error, "%s.%s: ", setting, property);
+				return FALSE;
 			}
-			g_hash_table_add (h, GUINT_TO_POINTER (v));
+			pvid_found = TRUE;
 		}
 	}