Merge pull request #5438 from sworleys/PBR-Fixes

pbrd: Assorted Fixes

lib/zclient.h

@@ -485,6 +485,29 @@ enum zapi_iptable_notify_owner {
 	ZAPI_IPTABLE_FAIL_REMOVE,
 };
 
+static inline const char *
+zapi_rule_notify_owner2str(enum zapi_rule_notify_owner note)
+{
+	const char *ret = "UNKNOWN";
+
+	switch (note) {
+	case ZAPI_RULE_FAIL_INSTALL:
+		ret = "ZAPI_RULE_FAIL_INSTALL";
+		break;
+	case ZAPI_RULE_INSTALLED:
+		ret = "ZAPI_RULE_INSTALLED";
+		break;
+	case ZAPI_RULE_FAIL_REMOVE:
+		ret = "ZAPI_RULE_FAIL_REMOVE";
+		break;
+	case ZAPI_RULE_REMOVED:
+		ret = "ZAPI_RULE_REMOVED";
+		break;
+	}
+
+	return ret;
+}
+
 /* Zebra MAC types */
 #define ZEBRA_MACIP_TYPE_STICKY                0x01 /* Sticky MAC*/
 #define ZEBRA_MACIP_TYPE_GW                    0x02 /* gateway (SVI) mac*/

pbrd/pbr_vty.c

@@ -127,11 +127,16 @@ DEFPY(pbr_map_match_src, pbr_map_match_src_cmd,
 	pbrms->family = prefix->family;
 
 	if (!no) {
-		if (prefix_same(pbrms->src, prefix))
+		if (pbrms->src) {
-			return CMD_SUCCESS;
+			if (prefix_same(pbrms->src, prefix))
+				return CMD_SUCCESS;
 
-		if (!pbrms->src)
+			vty_out(vty,
-			pbrms->src = prefix_new();
+				"A `match src-ip XX` command already exists, please remove that first\n");
+			return CMD_WARNING_CONFIG_FAILED;
+		}
+
+		pbrms->src = prefix_new();
 		prefix_copy(pbrms->src, prefix);
 	} else
 		prefix_free(&pbrms->src);
@@ -145,7 +150,7 @@ DEFPY(pbr_map_match_dst, pbr_map_match_dst_cmd,
 	"[no] match dst-ip <A.B.C.D/M|X:X::X:X/M>$prefix",
 	NO_STR
 	"Match the rest of the command\n"
-	"Choose the src ip or ipv6 prefix to use\n"
+	"Choose the dst ip or ipv6 prefix to use\n"
 	"v4 Prefix\n"
 	"v6 Prefix\n")
 {
@@ -154,11 +159,16 @@ DEFPY(pbr_map_match_dst, pbr_map_match_dst_cmd,
 	pbrms->family = prefix->family;
 
 	if (!no) {
-		if (prefix_same(pbrms->dst, prefix))
+		if (pbrms->dst) {
-			return CMD_SUCCESS;
+			if (prefix_same(pbrms->dst, prefix))
+				return CMD_SUCCESS;
 
-		if (!pbrms->dst)
+			vty_out(vty,
-			pbrms->dst = prefix_new();
+				"A `match dst-ip XX` command already exists, please remove that first\n");
+			return CMD_WARNING_CONFIG_FAILED;
+		}
+
+		pbrms->dst = prefix_new();
 		prefix_copy(pbrms->dst, prefix);
 	} else
 		prefix_free(&pbrms->dst);
@@ -183,12 +193,18 @@ DEFPY(pbr_map_match_mark, pbr_map_match_mark_cmd,
 #endif
 
 	if (!no) {
-		if (pbrms->mark == (uint32_t) mark)
+		if (pbrms->mark) {
-			return CMD_SUCCESS;
+			if (pbrms->mark == (uint32_t)mark)
-		pbrms->mark = (uint32_t) mark;
+				return CMD_SUCCESS;
-	} else {
+
+			vty_out(vty,
+				"A `match mark XX` command already exists, please remove that first\n");
+			return CMD_WARNING_CONFIG_FAILED;
+		}
+
+		pbrms->mark = (uint32_t)mark;
+	} else
 		pbrms->mark = 0;
-	}
 
 	pbr_map_check(pbrms);
 
@@ -232,7 +248,7 @@ DEFPY(pbr_map_nexthop_group, pbr_map_nexthop_group_cmd,
 			pbr_map_delete_nexthops(pbrms);
 		else {
 			vty_out(vty,
-				"Nexthop Group specified: %s does not exist to remove",
+				"Nexthop Group specified: %s does not exist to remove\n",
 				name);
 			return CMD_WARNING_CONFIG_FAILED;
 		}
@@ -240,7 +256,7 @@ DEFPY(pbr_map_nexthop_group, pbr_map_nexthop_group_cmd,
 		if (pbrms->nhgrp_name) {
 			if (strcmp(name, pbrms->nhgrp_name) != 0) {
 				vty_out(vty,
-					"Please delete current nexthop group before modifying current one");
+					"Please delete current nexthop group before modifying current one\n");
 				return CMD_WARNING_CONFIG_FAILED;
 			}
 
@@ -277,7 +293,7 @@ DEFPY(pbr_map_nexthop, pbr_map_nexthop_cmd,
 
 	if (pbrms->nhgrp_name) {
 		vty_out(vty,
-			"Please unconfigure the nexthop group before adding an individual nexthop");
+			"Please unconfigure the nexthop group before adding an individual nexthop\n");
 		return CMD_WARNING_CONFIG_FAILED;
 	}
 
@@ -359,7 +375,7 @@ DEFPY(pbr_map_nexthop, pbr_map_nexthop_cmd,
 
 		if (pbrms->nhg->nexthop) {
 			vty_out(vty,
-				"If you would like more than one nexthop please use nexthop-groups");
+				"If you would like more than one nexthop please use nexthop-groups\n");
 			return CMD_WARNING_CONFIG_FAILED;
 		}
 

pbrd/pbr_zebra.c

@@ -234,23 +234,21 @@ static int rule_notify_owner(ZAPI_CALLBACK_ARGS)
 	switch (note) {
 	case ZAPI_RULE_FAIL_INSTALL:
 		pbrms->installed &= ~installed;
-		DEBUGD(&pbr_dbg_zebra,
-		       "%s: Received RULE_FAIL_INSTALL: %" PRIu64,
-		       __PRETTY_FUNCTION__, pbrms->installed);
 		break;
 	case ZAPI_RULE_INSTALLED:
 		pbrms->installed |= installed;
-		DEBUGD(&pbr_dbg_zebra, "%s: Received RULE_INSTALLED: %" PRIu64,
-		       __PRETTY_FUNCTION__, pbrms->installed);
 		break;
 	case ZAPI_RULE_FAIL_REMOVE:
+		/* Don't change state on rule removal failure */
+		break;
 	case ZAPI_RULE_REMOVED:
 		pbrms->installed &= ~installed;
-		DEBUGD(&pbr_dbg_zebra, "%s: Received RULE REMOVED: %" PRIu64,
-		       __PRETTY_FUNCTION__, pbrms->installed);
 		break;
 	}
 
+	DEBUGD(&pbr_dbg_zebra, "%s: Received %s: %" PRIu64, __func__,
+	       zapi_rule_notify_owner2str(note), pbrms->installed);
+
 	pbr_map_final_interface_deletion(pbrms->parent, pmi);
 
 	return 0;