lldp: split parsing of LLDP attributes from lldp_neighbor_new()

Move the parsing of the LLDP attributes to a separate function.
In the next step, we will no longer keep all attribute around
and no longer parse them during lldp_neighbor_new().

One effect is that we can no longer (easily) declare the LLDP message as
invalid, if parsing the attributes fails. That makes IMO more sense,
because we should try to expose what little we could parse, and not
be forgiving to unexpected data. If we wanted, we still could hide such
neighbors entirely from being exposed, but that is not done, because
it seems better to expose the parts that were valid.

1 files changed, 113 insertions, 103 deletions

diff --git a/src/devices/nm-lldp-listener.c b/src/devices/nm-lldp-listener.c
index af0e13efda..7f44034e01 100644
--- a/src/devices/nm-lldp-listener.c
+++ b/src/devices/nm-lldp-listener.c
@@ -552,109 +552,32 @@ err:
 	return NULL;
 }
 
-static LldpNeighbor *
-lldp_neighbor_new (sd_lldp_neighbor *neighbor_sd, GError **error)
+static void
+_lldp_attrs_parse (LldpAttrs *attrs,
+                   sd_lldp_neighbor *neighbor_sd)
 {
-	nm_auto (lldp_neighbor_freep) LldpNeighbor *neigh = NULL;
-	uint8_t chassis_id_type, port_id_type;
+	const char *str;
 	uint16_t data16;
 	uint8_t *data8;
-	const void *chassis_id, *port_id;
-	gsize chassis_id_len, port_id_len, len;
-	const char *str;
+	gsize len;
 	int r;
 
-	r = sd_lldp_neighbor_get_chassis_id (neighbor_sd, &chassis_id_type,
-	                                     &chassis_id, &chassis_id_len);
-	if (r < 0) {
-		g_set_error (error, NM_UTILS_ERROR, NM_UTILS_ERROR_UNKNOWN,
-		             "failed reading chassis-id: %s", nm_strerror_native (-r));
-		return NULL;
-	}
-	if (chassis_id_len < 1) {
-		g_set_error (error, NM_UTILS_ERROR, NM_UTILS_ERROR_UNKNOWN,
-		             "empty chassis-id");
-		return NULL;
-	}
-
-	r = sd_lldp_neighbor_get_port_id (neighbor_sd, &port_id_type,
-	                                  &port_id, &port_id_len);
-	if (r < 0) {
-		g_set_error (error, NM_UTILS_ERROR, NM_UTILS_ERROR_UNKNOWN,
-		             "failed reading port-id: %s", nm_strerror_native (-r));
-		return NULL;
-	}
-	if (port_id_len < 1) {
-		g_set_error (error, NM_UTILS_ERROR, NM_UTILS_ERROR_UNKNOWN,
-		             "empty port-id");
-		return NULL;
-	}
-
-	neigh = g_slice_new (LldpNeighbor);
-	*neigh = (LldpNeighbor) {
-		.chassis_id_type = chassis_id_type,
-		.port_id_type    = port_id_type,
-		.neighbor_sd     = sd_lldp_neighbor_ref (neighbor_sd),
-	};
-
-	r = sd_lldp_neighbor_get_destination_address (neighbor_sd, &neigh->destination_address);
-	if (r < 0) {
-		g_set_error (error, NM_UTILS_ERROR, NM_UTILS_ERROR_UNKNOWN,
-		             "failed getting destination address: %s", nm_strerror_native (-r));
-		goto out;
-	}
-
-	switch (chassis_id_type) {
-	case SD_LLDP_CHASSIS_SUBTYPE_INTERFACE_ALIAS:
-	case SD_LLDP_CHASSIS_SUBTYPE_INTERFACE_NAME:
-	case SD_LLDP_CHASSIS_SUBTYPE_LOCALLY_ASSIGNED:
-	case SD_LLDP_CHASSIS_SUBTYPE_CHASSIS_COMPONENT:
-		neigh->chassis_id =    nm_utils_buf_utf8safe_escape_cp (chassis_id, chassis_id_len, NM_UTILS_STR_UTF8_SAFE_FLAG_ESCAPE_CTRL | NM_UTILS_STR_UTF8_SAFE_FLAG_ESCAPE_NON_ASCII)
-		                    ?: g_new0 (char, 1);
-		break;
-	case SD_LLDP_CHASSIS_SUBTYPE_MAC_ADDRESS:
-		neigh->chassis_id = nm_utils_hwaddr_ntoa (chassis_id, chassis_id_len);
-		break;
-	default:
-		g_set_error (error, NM_UTILS_ERROR, NM_UTILS_ERROR_UNKNOWN,
-		             "unsupported chassis-id type %d", chassis_id_type);
-		goto out;
-	}
-
-	switch (port_id_type) {
-	case SD_LLDP_PORT_SUBTYPE_INTERFACE_ALIAS:
-	case SD_LLDP_PORT_SUBTYPE_INTERFACE_NAME:
-	case SD_LLDP_PORT_SUBTYPE_LOCALLY_ASSIGNED:
-	case SD_LLDP_PORT_SUBTYPE_PORT_COMPONENT:
-		neigh->port_id =    nm_utils_buf_utf8safe_escape_cp (port_id, port_id_len, NM_UTILS_STR_UTF8_SAFE_FLAG_ESCAPE_CTRL | NM_UTILS_STR_UTF8_SAFE_FLAG_ESCAPE_NON_ASCII)
-		                 ?: g_new0 (char, 1);
-		break;
-	case SD_LLDP_PORT_SUBTYPE_MAC_ADDRESS:
-		neigh->port_id = nm_utils_hwaddr_ntoa (port_id, port_id_len);
-		break;
-	default:
-		g_set_error (error, NM_UTILS_ERROR, NM_UTILS_ERROR_UNKNOWN,
-		             "unsupported port-id type %d", port_id_type);
-		goto out;
-	}
-
 	if (sd_lldp_neighbor_get_port_description (neighbor_sd, &str) == 0)
-		_lldp_attrs_set_str (&neigh->attrs, LLDP_ATTR_ID_PORT_DESCRIPTION, str);
+		_lldp_attrs_set_str (attrs, LLDP_ATTR_ID_PORT_DESCRIPTION, str);
 
 	if (sd_lldp_neighbor_get_system_name (neighbor_sd, &str) == 0)
-		_lldp_attrs_set_str (&neigh->attrs, LLDP_ATTR_ID_SYSTEM_NAME, str);
+		_lldp_attrs_set_str (attrs, LLDP_ATTR_ID_SYSTEM_NAME, str);
 
 	if (sd_lldp_neighbor_get_system_description (neighbor_sd, &str) == 0)
-		_lldp_attrs_set_str (&neigh->attrs, LLDP_ATTR_ID_SYSTEM_DESCRIPTION, str);
+		_lldp_attrs_set_str (attrs, LLDP_ATTR_ID_SYSTEM_DESCRIPTION, str);
 
 	if (sd_lldp_neighbor_get_system_capabilities (neighbor_sd, &data16) == 0)
-		_lldp_attrs_set_uint32 (&neigh->attrs, LLDP_ATTR_ID_SYSTEM_CAPABILITIES, data16);
+		_lldp_attrs_set_uint32 (attrs, LLDP_ATTR_ID_SYSTEM_CAPABILITIES, data16);
 
 	r = sd_lldp_neighbor_tlv_rewind (neighbor_sd);
 	if (r < 0) {
-		g_set_error (error, NM_UTILS_ERROR, NM_UTILS_ERROR_UNKNOWN,
-		             "failed reading tlv (rewind): %s", nm_strerror_native (-r));
-		goto out;
+		nm_assert_not_reached ();
+		return;
 	}
 	do {
 		guint8 oui[3];
@@ -671,7 +594,7 @@ lldp_neighbor_new (sd_lldp_neighbor *neighbor_sd, GError **error)
 		case SD_LLDP_TYPE_MGMT_ADDRESS:
 			variant = parse_management_address_tlv (data8, len);
 			if (variant) {
-				_lldp_attr_add_variant (&neigh->attrs,
+				_lldp_attr_add_variant (attrs,
 				                        LLDP_ATTR_ID_MANAGEMENT_ADDRESSES,
 				                        variant);
 			}
@@ -686,9 +609,10 @@ lldp_neighbor_new (sd_lldp_neighbor *neighbor_sd, GError **error)
 		if (r < 0) {
 			if (r == -ENXIO)
 				continue;
-			g_set_error (error, NM_UTILS_ERROR, NM_UTILS_ERROR_UNKNOWN,
-			             "failed reading tlv: %s", nm_strerror_native (-r));
-			goto out;
+
+			/* in other cases, something is seriously wrong. Abort, but
+			 * keep what we parsed so far. */
+			return;
 		}
 
 		if (   memcmp (oui, SD_LLDP_OUI_802_1, sizeof (oui)) != 0
@@ -720,22 +644,22 @@ lldp_neighbor_new (sd_lldp_neighbor *neighbor_sd, GError **error)
 			case SD_LLDP_OUI_802_1_SUBTYPE_PORT_VLAN_ID:
 				if (len != 2)
 					continue;
-				_lldp_attrs_set_uint32 (&neigh->attrs, LLDP_ATTR_ID_IEEE_802_1_PVID,
+				_lldp_attrs_set_uint32 (attrs, LLDP_ATTR_ID_IEEE_802_1_PVID,
 				                        unaligned_read_be16 (data8));
 				break;
 			case SD_LLDP_OUI_802_1_SUBTYPE_PORT_PROTOCOL_VLAN_ID:
 				if (len != 3)
 					continue;
-				_lldp_attrs_set_uint32 (&neigh->attrs, LLDP_ATTR_ID_IEEE_802_1_PPVID_FLAGS,
+				_lldp_attrs_set_uint32 (attrs, LLDP_ATTR_ID_IEEE_802_1_PPVID_FLAGS,
 				                        data8[0]);
-				_lldp_attrs_set_uint32 (&neigh->attrs, LLDP_ATTR_ID_IEEE_802_1_PPVID,
+				_lldp_attrs_set_uint32 (attrs, LLDP_ATTR_ID_IEEE_802_1_PPVID,
 				                        unaligned_read_be16 (&data8[1]));
 
 				g_variant_dict_init (&dict, NULL);
 				g_variant_dict_insert (&dict, "ppvid", "u", (guint32) unaligned_read_be16 (&data8[1]));
 				g_variant_dict_insert (&dict, "flags", "u", (guint32) data8[0]);
 
-				_lldp_attr_add_variant (&neigh->attrs,
+				_lldp_attr_add_variant (attrs,
 				                        LLDP_ATTR_ID_IEEE_802_1_PPVIDS,
 				                        g_variant_dict_end (&dict));
 				break;
@@ -761,15 +685,15 @@ lldp_neighbor_new (sd_lldp_neighbor *neighbor_sd, GError **error)
 				g_variant_dict_insert (&dict, "vid", "u", vid);
 				g_variant_dict_insert (&dict, "name", "s", name);
 
-				_lldp_attr_add_variant (&neigh->attrs,
+				_lldp_attr_add_variant (attrs,
 				                        LLDP_ATTR_ID_IEEE_802_1_VLANS,
 				                        g_variant_dict_end (&dict));
 
-				_lldp_attrs_set_uint32 (&neigh->attrs, LLDP_ATTR_ID_IEEE_802_1_VID, vid);
+				_lldp_attrs_set_uint32 (attrs, LLDP_ATTR_ID_IEEE_802_1_VID, vid);
 				if (name_to_free)
-					_lldp_attrs_set_str_take (&neigh->attrs, LLDP_ATTR_ID_IEEE_802_1_VLAN_NAME, name_to_free);
+					_lldp_attrs_set_str_take (attrs, LLDP_ATTR_ID_IEEE_802_1_VLAN_NAME, name_to_free);
 				else
-					_lldp_attrs_set_str (&neigh->attrs, LLDP_ATTR_ID_IEEE_802_1_VLAN_NAME, name);
+					_lldp_attrs_set_str (attrs, LLDP_ATTR_ID_IEEE_802_1_VLAN_NAME, name);
 				break;
 			}
 			default:
@@ -788,7 +712,7 @@ lldp_neighbor_new (sd_lldp_neighbor *neighbor_sd, GError **error)
 				g_variant_dict_insert (&dict, "pmd-autoneg-cap", "u", (guint32) unaligned_read_be16 (&data8[1]));
 				g_variant_dict_insert (&dict, "operational-mau-type", "u", (guint32) unaligned_read_be16 (&data8[3]));
 
-				_lldp_attrs_set_variant (&neigh->attrs,
+				_lldp_attrs_set_variant (attrs,
 				                         LLDP_ATTR_ID_IEEE_802_3_MAC_PHY_CONF,
 				                         g_variant_dict_end (&dict));
 				break;
@@ -801,20 +725,106 @@ lldp_neighbor_new (sd_lldp_neighbor *neighbor_sd, GError **error)
 				g_variant_dict_insert (&dict, "pse-power-pair", "u", (guint32) data8[1]);
 				g_variant_dict_insert (&dict, "power-class", "u", (guint32) data8[2]);
 
-				_lldp_attrs_set_variant (&neigh->attrs,
+				_lldp_attrs_set_variant (attrs,
 				                         LLDP_ATTR_ID_IEEE_802_3_POWER_VIA_MDI,
 				                         g_variant_dict_end (&dict));
 				break;
 			case SD_LLDP_OUI_802_3_SUBTYPE_MAXIMUM_FRAME_SIZE:
 				if (len != 2)
 					continue;
-				_lldp_attrs_set_uint32 (&neigh->attrs,
+				_lldp_attrs_set_uint32 (attrs,
 				                        LLDP_ATTR_ID_IEEE_802_3_MAX_FRAME_SIZE,
 				                        unaligned_read_be16 (data8));
 				break;
 			}
 		}
 	} while (sd_lldp_neighbor_tlv_next (neighbor_sd) > 0);
+}
+
+static LldpNeighbor *
+lldp_neighbor_new (sd_lldp_neighbor *neighbor_sd, GError **error)
+{
+	nm_auto (lldp_neighbor_freep) LldpNeighbor *neigh = NULL;
+	uint8_t chassis_id_type, port_id_type;
+	const void *chassis_id, *port_id;
+	gsize chassis_id_len, port_id_len;
+	int r;
+
+	r = sd_lldp_neighbor_get_chassis_id (neighbor_sd, &chassis_id_type,
+	                                     &chassis_id, &chassis_id_len);
+	if (r < 0) {
+		g_set_error (error, NM_UTILS_ERROR, NM_UTILS_ERROR_UNKNOWN,
+		             "failed reading chassis-id: %s", nm_strerror_native (-r));
+		return NULL;
+	}
+	if (chassis_id_len < 1) {
+		g_set_error (error, NM_UTILS_ERROR, NM_UTILS_ERROR_UNKNOWN,
+		             "empty chassis-id");
+		return NULL;
+	}
+
+	r = sd_lldp_neighbor_get_port_id (neighbor_sd, &port_id_type,
+	                                  &port_id, &port_id_len);
+	if (r < 0) {
+		g_set_error (error, NM_UTILS_ERROR, NM_UTILS_ERROR_UNKNOWN,
+		             "failed reading port-id: %s", nm_strerror_native (-r));
+		return NULL;
+	}
+	if (port_id_len < 1) {
+		g_set_error (error, NM_UTILS_ERROR, NM_UTILS_ERROR_UNKNOWN,
+		             "empty port-id");
+		return NULL;
+	}
+
+	neigh = g_slice_new (LldpNeighbor);
+	*neigh = (LldpNeighbor) {
+		.chassis_id_type = chassis_id_type,
+		.port_id_type    = port_id_type,
+		.neighbor_sd     = sd_lldp_neighbor_ref (neighbor_sd),
+	};
+
+	r = sd_lldp_neighbor_get_destination_address (neighbor_sd, &neigh->destination_address);
+	if (r < 0) {
+		g_set_error (error, NM_UTILS_ERROR, NM_UTILS_ERROR_UNKNOWN,
+		             "failed getting destination address: %s", nm_strerror_native (-r));
+		goto out;
+	}
+
+	switch (chassis_id_type) {
+	case SD_LLDP_CHASSIS_SUBTYPE_INTERFACE_ALIAS:
+	case SD_LLDP_CHASSIS_SUBTYPE_INTERFACE_NAME:
+	case SD_LLDP_CHASSIS_SUBTYPE_LOCALLY_ASSIGNED:
+	case SD_LLDP_CHASSIS_SUBTYPE_CHASSIS_COMPONENT:
+		neigh->chassis_id =    nm_utils_buf_utf8safe_escape_cp (chassis_id, chassis_id_len, NM_UTILS_STR_UTF8_SAFE_FLAG_ESCAPE_CTRL | NM_UTILS_STR_UTF8_SAFE_FLAG_ESCAPE_NON_ASCII)
+		                    ?: g_new0 (char, 1);
+		break;
+	case SD_LLDP_CHASSIS_SUBTYPE_MAC_ADDRESS:
+		neigh->chassis_id = nm_utils_hwaddr_ntoa (chassis_id, chassis_id_len);
+		break;
+	default:
+		g_set_error (error, NM_UTILS_ERROR, NM_UTILS_ERROR_UNKNOWN,
+		             "unsupported chassis-id type %d", chassis_id_type);
+		goto out;
+	}
+
+	switch (port_id_type) {
+	case SD_LLDP_PORT_SUBTYPE_INTERFACE_ALIAS:
+	case SD_LLDP_PORT_SUBTYPE_INTERFACE_NAME:
+	case SD_LLDP_PORT_SUBTYPE_LOCALLY_ASSIGNED:
+	case SD_LLDP_PORT_SUBTYPE_PORT_COMPONENT:
+		neigh->port_id =    nm_utils_buf_utf8safe_escape_cp (port_id, port_id_len, NM_UTILS_STR_UTF8_SAFE_FLAG_ESCAPE_CTRL | NM_UTILS_STR_UTF8_SAFE_FLAG_ESCAPE_NON_ASCII)
+		                 ?: g_new0 (char, 1);
+		break;
+	case SD_LLDP_PORT_SUBTYPE_MAC_ADDRESS:
+		neigh->port_id = nm_utils_hwaddr_ntoa (port_id, port_id_len);
+		break;
+	default:
+		g_set_error (error, NM_UTILS_ERROR, NM_UTILS_ERROR_UNKNOWN,
+		             "unsupported port-id type %d", port_id_type);
+		goto out;
+	}
+
+	_lldp_attrs_parse (&neigh->attrs, neighbor_sd);
 
 	neigh->valid = TRUE;