pbrd: use flags to indicate active fields

    Before now, PBRD used non-zero values to imply that a rule's
    match or action field was active. This approach was getting
    cumbersome for fields where 0 is a valid active value and
    various field-specific magic values had to be used.

    This commit changes PBRD to use a flag bit per field to
    indicate that the field is active.

Signed-off-by: G. Paul Ziemba <paulz@labn.net>

5 files changed, 602 insertions, 343 deletions

diff --git a/pbrd/pbr_map.c b/pbrd/pbr_map.c
index 758e08b56..30148fe09 100644
--- a/pbrd/pbr_map.c
+++ b/pbrd/pbr_map.c
@@ -105,38 +105,6 @@ static bool pbrms_is_installed(const struct pbr_map_sequence *pbrms,
 	return false;
 }
 
-void pbr_set_match_clause_for_pcp(struct pbr_map_sequence *pbrms, bool set,
-				  uint8_t pcp)
-{
-	bool changed = false;
-
-	if (set) {
-		if (!CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_PCP) ||
-		    (pcp != pbrms->match_pcp)) {
-			SET_FLAG(pbrms->filter_bm, PBR_FILTER_PCP);
-			pbrms->match_pcp = pcp;
-			changed = true;
-		}
-	} else {
-		if (CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_PCP)) {
-			UNSET_FLAG(pbrms->filter_bm, PBR_FILTER_PCP);
-			changed = true;
-		}
-	}
-	if (changed)
-		pbr_map_check(pbrms, true);
-}
-
-void pbr_set_match_clause_for_vlan(struct pbr_map_sequence *pbrms,
-				   uint16_t vlan_id, uint16_t vlan_flags)
-{
-	if (pbrms) {
-		pbrms->match_vlan_id = vlan_id;
-		pbrms->match_vlan_flags = vlan_flags;
-		pbr_map_check(pbrms, true);
-	}
-}
-
 /* If any sequence is installed on the interface, assume installed */
 static bool
 pbr_map_interface_is_installed(const struct pbr_map *pbrm,
@@ -562,14 +530,6 @@ struct pbr_map_sequence *pbrms_get(const char *name, uint32_t seqno)
 		pbrms->ruleno = pbr_nht_get_next_rule(seqno);
 		pbrms->parent = pbrm;
 
-		pbrms->match_vlan_id = 0;
-		pbrms->match_vlan_flags = 0;
-		pbrms->match_pcp = 0;
-
-		pbrms->action_vlan_id = 0;
-		pbrms->action_vlan_flags = 0;
-		pbrms->action_pcp = 0;
-
 		pbrms->action_queue_id = PBR_MAP_UNDEFINED_QUEUE_ID;
 
 		pbrms->reason =
@@ -634,13 +594,35 @@ pbr_map_sequence_check_nexthops_valid(struct pbr_map_sequence *pbrms)
 
 static void pbr_map_sequence_check_not_empty(struct pbr_map_sequence *pbrms)
 {
-	if (!pbrms->src && !pbrms->dst && !pbrms->mark && !pbrms->dsfield &&
-	    !CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_PCP) &&
-	    !pbrms->action_pcp && !pbrms->match_vlan_id &&
-	    !pbrms->match_vlan_flags && !pbrms->action_vlan_id &&
-	    !pbrms->action_vlan_flags &&
-	    pbrms->action_queue_id == PBR_MAP_UNDEFINED_QUEUE_ID)
+	/* clang-format off */
+	if (
+		!CHECK_FLAG(pbrms->filter_bm, (
+			PBR_FILTER_SRC_IP |
+			PBR_FILTER_DST_IP |
+			PBR_FILTER_SRC_PORT |
+			PBR_FILTER_DST_PORT |
+
+			PBR_FILTER_IP_PROTOCOL |
+			PBR_FILTER_DSCP |
+			PBR_FILTER_ECN |
+
+			PBR_FILTER_FWMARK |
+			PBR_FILTER_PCP |
+			PBR_FILTER_VLAN_ID |
+			PBR_FILTER_VLAN_FLAGS
+		)) &&
+		!CHECK_FLAG(pbrms->action_bm, (
+
+			PBR_ACTION_PCP |
+			PBR_ACTION_VLAN_ID |
+			PBR_ACTION_VLAN_STRIP_INNER_ANY |
+
+			PBR_ACTION_QUEUE_ID
+		))
+	) {
 		pbrms->reason |= PBR_MAP_INVALID_EMPTY;
+	}
+	/* clang-format on */
 }
 
 static void pbr_map_sequence_check_vlan_actions(struct pbr_map_sequence *pbrms)
@@ -653,7 +635,8 @@ static void pbr_map_sequence_check_vlan_actions(struct pbr_map_sequence *pbrms)
 	 * The strip vlan action removes any inner tag, so it is invalid to
 	 * specify both a set and strip action.
 	 */
-	if ((pbrms->action_vlan_id != 0) && (pbrms->action_vlan_flags != 0))
+	if (CHECK_FLAG(pbrms->action_bm, PBR_ACTION_VLAN_ID) &&
+	    (CHECK_FLAG(pbrms->action_bm, PBR_ACTION_VLAN_STRIP_INNER_ANY)))
 		pbrms->reason |= PBR_MAP_INVALID_SET_STRIP_VLAN;
 }
 
diff --git a/pbrd/pbr_map.h b/pbrd/pbr_map.h
index 3afb19956..700bc93f7 100644
--- a/pbrd/pbr_map.h
+++ b/pbrd/pbr_map.h
@@ -3,7 +3,9 @@
  * PBR-map Header
  * Copyright (C) 2018 Cumulus Networks, Inc.
  *               Donald Sharp
- * Copyright (c) 2023 LabN Consulting, L.L.C.
+ * Portions:
+ *	Copyright (c) 2023 LabN Consulting, L.L.C.
+ *	Copyright (c) 2021 The MITRE Corporation
  */
 #ifndef __PBR_MAP_H__
 #define __PBR_MAP_H__
@@ -54,6 +56,14 @@ struct pbr_map_interface {
 	bool delete;
 };
 
+enum pbr_forwarding_type {
+	PBR_FT_UNSPEC = 0,
+	PBR_FT_VRF_UNCHANGED,
+	PBR_FT_SETVRF,
+	PBR_FT_NEXTHOP_GROUP,
+	PBR_FT_NEXTHOP_SINGLE,
+};
+
 struct pbr_map_sequence {
 	struct pbr_map *parent;
 
@@ -109,14 +119,19 @@ struct pbr_map_sequence {
 	 * Action fields
 	 *****************************************************************/
 
+	/*
+	 * same bit definitions as in lib/pbr.h
+	 */
+	uint32_t action_bm;
+
 	uint8_t action_pcp;
 	uint8_t action_vlan_id;
-#define PBR_MAP_STRIP_INNER_ANY (1 << 0)
-	uint8_t action_vlan_flags;
 
 #define PBR_MAP_UNDEFINED_QUEUE_ID 0
 	uint32_t action_queue_id;
 
+	enum pbr_forwarding_type forwarding_type;
+
 	/*
 	 * Use interface's vrf.
 	 */
@@ -233,9 +248,4 @@ extern void pbr_map_check_vrf_nh_group_change(const char *nh_group,
 extern void pbr_map_check_interface_nh_group_change(const char *nh_group,
 						    struct interface *ifp,
 						    ifindex_t oldifindex);
-extern void pbr_set_match_clause_for_vlan(struct pbr_map_sequence *pbrms,
-					  uint16_t vlan_id,
-					  uint16_t vlan_flags);
-extern void pbr_set_match_clause_for_pcp(struct pbr_map_sequence *pbrms,
-					 bool set, uint8_t pcp);
 #endif
diff --git a/pbrd/pbr_nht.c b/pbrd/pbr_nht.c
index 405a2d658..4f7882fb2 100644
--- a/pbrd/pbr_nht.c
+++ b/pbrd/pbr_nht.c
@@ -549,6 +549,7 @@ void pbr_nht_set_seq_nhg(struct pbr_map_sequence *pbrms, const char *name)
 		return;
 
 	pbrms->nhgrp_name = XSTRDUP(MTYPE_TMP, name);
+	pbrms->forwarding_type = PBR_FT_NEXTHOP_GROUP;
 
 	nhgc = nhgc_find(name);
 	if (!nhgc)
@@ -572,6 +573,7 @@ void pbr_nht_add_individual_nexthop(struct pbr_map_sequence *pbrms,
 		MTYPE_TMP,
 		pbr_nht_nexthop_make_name(pbrms->parent->name, PBR_NHC_NAMELEN,
 					  pbrms->seqno, buf));
+	pbrms->forwarding_type = PBR_FT_NEXTHOP_SINGLE;
 
 	nh = nexthop_new();
 	memcpy(nh, nhop, sizeof(*nh));
diff --git a/pbrd/pbr_vty.c b/pbrd/pbr_vty.c
index cffd3794c..0d8fa6939 100644
--- a/pbrd/pbr_vty.c
+++ b/pbrd/pbr_vty.c
@@ -132,10 +132,78 @@ DEFUN_NOSH(no_pbr_map,
  *		pbrms/rule Match L3 Fields
  ***********************************************************************/
 
+/*
+ * Address Family Matters
+ *
+ * Linux Kernel constraints
+ * ------------------------
+ * The underlying linux kernel dataplane requires that rules be
+ * installed into an IPv4-specific or an IPv6-specific database.
+ *
+ * Not only do we need to designate an address-family for rule
+ * installation, but we ALSO must have the same address-family
+ * available to be able to delete the rule from the correct kernel
+ * database.
+ *
+ * Determining the address-family
+ * ------------------------------
+ * In the current code, we do our best to infer the correct family
+ * from any configured IP-address match or set clauses in a rule.
+ * Absent any of those fields, the NHT code also tries to glean the
+ * address family from resolved nexthops or nexthop-groups. All of
+ * those opportunistic address-family determinations are stored in
+ * the "family" field of struct pbr_map_sequence.
+ *
+ * This "family" field value is needed particularly when deleting
+ * a rule piece-by-piece because at the end, the match/set fields
+ * will be empty. Maybe it would be possible to handle this issue
+ * as an internal zebra matter in the future.
+ *
+ * We also attempt to maintain address-family consistency among the
+ * various configured fields in a rule. So far, these fields are
+ * src/dst IP-address match/set values.
+ *
+ * It is probably possible to perform the same address-family check in
+ * the CLI for single nexthops (set nexthop A.B.C.D|X:X::X:X) but the
+ * address-family is not immediately available for nexthop-groups.
+ * In both the single-nexthop and nexthop-group, the NHT resolution code
+ * sets the "family" field of struct pbr_map_sequence asynchronously.
+ *
+ * There isn't currently any flagging of rules that have a consistent
+ * set of src/dst IP-address match/set values but an asynchronously-resolved
+ * nexthop-group that has a different address-family.
+ *
+ * The match/set IP-address handlers below blindly set "family"; it's
+ * probably possible to wrongly set "family" to, e.g., IPv4 this way after
+ * a v6 NHG has been resolved and break rule removal. It's not clear
+ * how to best address this potential issue.
+ */
+static bool pbr_family_consistent(struct pbr_map_sequence *pbrms,
+				  uint8_t family, uint32_t skip_filter_bm,
+				  uint32_t skip_action_bm, const char **msg)
+{
+	uint32_t filter_bm = pbrms->filter_bm & ~skip_filter_bm;
+
+	if (CHECK_FLAG(filter_bm, PBR_FILTER_SRC_IP) &&
+	    (family != pbrms->src->family)) {
+		if (msg)
+			*msg = "match src-ip";
+		return false;
+	}
+	if (CHECK_FLAG(filter_bm, PBR_FILTER_DST_IP) &&
+	    (family != pbrms->dst->family)) {
+		if (msg)
+			*msg = "match dst-ip";
+		return false;
+	}
+	return true;
+}
+
+
 /* clang-format off */
 DEFPY  (pbr_map_match_src,
 	pbr_map_match_src_cmd,
-	"[no] match src-ip <A.B.C.D/M|X:X::X:X/M>$prefix",
+	"[no] match src-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"
@@ -144,28 +212,37 @@ DEFPY  (pbr_map_match_src,
 {
 	/* clang-format on */
 	struct pbr_map_sequence *pbrms = VTY_GET_CONTEXT(pbr_map_sequence);
+	const char *fmsg = NULL;
 
 	if (!pbrms)
 		return CMD_WARNING_CONFIG_FAILED;
 
-	if (pbrms->dst && prefix->family != pbrms->dst->family) {
-		vty_out(vty, "Cannot mismatch families within match src/dst\n");
-		return CMD_WARNING_CONFIG_FAILED;
+	if (no) {
+		if (!CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_SRC_IP))
+			return CMD_SUCCESS;
+		prefix_free(&pbrms->src);
+		UNSET_FLAG(pbrms->filter_bm, PBR_FILTER_SRC_IP);
+		goto check;
 	}
 
+	assert(prefix);
+	if (!pbr_family_consistent(pbrms, prefix->family, PBR_FILTER_SRC_IP, 0,
+				   &fmsg)) {
+		vty_out(vty, "Address family mismatch (%s)\n", fmsg);
+		return CMD_WARNING_CONFIG_FAILED;
+	}
 	pbrms->family = prefix->family;
 
-	if (!no) {
-		if (pbrms->src) {
-			if (prefix_same(pbrms->src, prefix))
-				return CMD_SUCCESS;
-		} else
-			pbrms->src = prefix_new();
-
-		prefix_copy(pbrms->src, prefix);
+	if (CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_SRC_IP)) {
+		if (prefix_same(pbrms->src, prefix))
+			return CMD_SUCCESS;
 	} else
-		prefix_free(&pbrms->src);
+		pbrms->src = prefix_new();
+
+	prefix_copy(pbrms->src, prefix);
+	SET_FLAG(pbrms->filter_bm, PBR_FILTER_SRC_IP);
 
+check:
 	pbr_map_check(pbrms, true);
 
 	return CMD_SUCCESS;
@@ -174,7 +251,7 @@ DEFPY  (pbr_map_match_src,
 /* clang-format off */
 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] 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 dst ip or ipv6 prefix to use\n"
@@ -183,27 +260,37 @@ DEFPY  (pbr_map_match_dst,
 {
 	/* clang-format on */
 	struct pbr_map_sequence *pbrms = VTY_GET_CONTEXT(pbr_map_sequence);
+	const char *fmsg = NULL;
 
 	if (!pbrms)
 		return CMD_WARNING_CONFIG_FAILED;
 
-	if (pbrms->src && prefix->family != pbrms->src->family) {
-		vty_out(vty, "Cannot mismatch families within match src/dst\n");
+	if (no) {
+		if (!CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_DST_IP))
+			return CMD_SUCCESS;
+		prefix_free(&pbrms->dst);
+		UNSET_FLAG(pbrms->filter_bm, PBR_FILTER_DST_IP);
+		goto check;
+	}
+
+	assert(prefix);
+	if (!pbr_family_consistent(pbrms, prefix->family, PBR_FILTER_DST_IP, 0,
+				   &fmsg)) {
+		vty_out(vty, "Address family mismatch (%s)\n", fmsg);
 		return CMD_WARNING_CONFIG_FAILED;
 	}
 	pbrms->family = prefix->family;
 
-	if (!no) {
-		if (pbrms->dst) {
-			if (prefix_same(pbrms->dst, prefix))
-				return CMD_SUCCESS;
-		} else
-			pbrms->dst = prefix_new();
-
-		prefix_copy(pbrms->dst, prefix);
+	if (CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_DST_IP)) {
+		if (prefix_same(pbrms->dst, prefix))
+			return CMD_SUCCESS;
 	} else
-		prefix_free(&pbrms->dst);
+		pbrms->dst = prefix_new();
 
+	prefix_copy(pbrms->dst, prefix);
+	SET_FLAG(pbrms->filter_bm, PBR_FILTER_DST_IP);
+
+check:
 	pbr_map_check(pbrms, true);
 
 	return CMD_SUCCESS;
@@ -212,7 +299,7 @@ DEFPY  (pbr_map_match_dst,
 /* clang-format off */
 DEFPY  (pbr_map_match_ip_proto,
 	pbr_map_match_ip_proto_cmd,
-	"[no] match ip-protocol PROTO$ip_proto",
+	"[no] match ip-protocol ![PROTO$ip_proto]",
 	NO_STR
 	"Match the rest of the command\n"
 	"Choose an ip-protocol\n"
@@ -220,37 +307,39 @@ DEFPY  (pbr_map_match_ip_proto,
 {
 	/* clang-format on */
 	struct pbr_map_sequence *pbrms = VTY_GET_CONTEXT(pbr_map_sequence);
-	struct protoent *p;
+	struct protoent *p = NULL;
 
 	if (!pbrms)
 		return CMD_WARNING_CONFIG_FAILED;
 
-	if (!no) {
-		if (!ip_proto) {
-			vty_out(vty, "Unable to convert (null) to proto id\n");
-			return CMD_WARNING;
-		}
+	if (no) {
+		if (!CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_IP_PROTOCOL))
+			return CMD_SUCCESS;
+		UNSET_FLAG(pbrms->filter_bm, PBR_FILTER_IP_PROTOCOL);
+		goto check;
+	}
 
+	if (ip_proto)
 		p = getprotobyname(ip_proto);
-		if (!p) {
-			vty_out(vty, "Unable to convert %s to proto id\n",
-				ip_proto);
-			return CMD_WARNING;
-		}
 
-		pbrms->ip_proto = p->p_proto;
-	} else
-		pbrms->ip_proto = 0;
+	if (!ip_proto || !p) {
+		vty_out(vty, "Unable to convert %s to proto id\n",
+			(ip_proto ? ip_proto : "(null)"));
+		return CMD_WARNING_CONFIG_FAILED;
+	}
 
-	pbr_map_check(pbrms, true);
+	pbrms->ip_proto = p->p_proto;
+	SET_FLAG(pbrms->filter_bm, PBR_FILTER_IP_PROTOCOL);
 
+check:
+	pbr_map_check(pbrms, true);
 	return CMD_SUCCESS;
 }
 
 /* clang-format off */
 DEFPY  (pbr_map_match_src_port,
 	pbr_map_match_src_port_cmd,
-	"[no] match src-port (1-65535)$port",
+	"[no] match src-port ![(1-65535)$port]",
 	NO_STR
 	"Match the rest of the command\n"
 	"Choose the source port to use\n"
@@ -262,23 +351,29 @@ DEFPY  (pbr_map_match_src_port,
 	if (!pbrms)
 		return CMD_WARNING_CONFIG_FAILED;
 
-	if (!no) {
-		if (pbrms->src_prt == port)
+	if (no) {
+		if (!CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_SRC_PORT))
 			return CMD_SUCCESS;
-		else
-			pbrms->src_prt = port;
-	} else
-		pbrms->src_prt = 0;
+		UNSET_FLAG(pbrms->filter_bm, PBR_FILTER_SRC_PORT);
+		goto check;
+	}
 
-	pbr_map_check(pbrms, true);
+	if (CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_SRC_PORT) &&
+	    (pbrms->src_prt == port)) {
+		return CMD_SUCCESS;
+	}
+	pbrms->src_prt = port;
+	SET_FLAG(pbrms->filter_bm, PBR_FILTER_SRC_PORT);
 
+check:
+	pbr_map_check(pbrms, true);
 	return CMD_SUCCESS;
 }
 
 /* clang-format off */
 DEFPY  (pbr_map_match_dst_port,
 	pbr_map_match_dst_port_cmd,
-	"[no] match dst-port (1-65535)$port",
+	"[no] match dst-port ![(1-65535)$port]",
 	NO_STR
 	"Match the rest of the command\n"
 	"Choose the destination port to use\n"
@@ -290,23 +385,29 @@ DEFPY  (pbr_map_match_dst_port,
 	if (!pbrms)
 		return CMD_WARNING_CONFIG_FAILED;
 
-	if (!no) {
-		if (pbrms->dst_prt == port)
+	if (no) {
+		if (!CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_DST_PORT))
 			return CMD_SUCCESS;
-		else
-			pbrms->dst_prt = port;
-	} else
-		pbrms->dst_prt = 0;
+		UNSET_FLAG(pbrms->filter_bm, PBR_FILTER_DST_PORT);
+		goto check;
+	}
 
-	pbr_map_check(pbrms, true);
+	if (CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_DST_PORT) &&
+	    (pbrms->dst_prt == port)) {
+		return CMD_SUCCESS;
+	}
+	pbrms->dst_prt = port;
+	SET_FLAG(pbrms->filter_bm, PBR_FILTER_DST_PORT);
 
+check:
+	pbr_map_check(pbrms, true);
 	return CMD_SUCCESS;
 }
 
 /* clang-format off */
 DEFPY  (pbr_map_match_dscp,
 	pbr_map_match_dscp_cmd,
-	"[no] match dscp DSCP$dscp",
+	"[no] match dscp ![DSCP$dscp]",
 	NO_STR
 	"Match the rest of the command\n"
 	"Match based on IP DSCP field\n"
@@ -314,68 +415,52 @@ DEFPY  (pbr_map_match_dscp,
 {
 	/* clang-format on */
 	struct pbr_map_sequence *pbrms = VTY_GET_CONTEXT(pbr_map_sequence);
-	char dscpname[100];
-	uint8_t rawDscp;
 
 	if (!pbrms)
 		return CMD_WARNING_CONFIG_FAILED;
 
-	/* Discriminate dscp enums (cs0, cs1 etc.) and numbers */
-	bool isANumber = true;
-	for (int i = 0; i < (int)strlen(dscp); i++) {
-		/* Letters are not numbers */
-		if (!isdigit(dscp[i]))
-			isANumber = false;
-
-		/* Lowercase the dscp enum (if needed) */
-		if (isupper(dscp[i]))
-			dscpname[i] = tolower(dscp[i]);
-		else
-			dscpname[i] = dscp[i];
+	if (no) {
+		if (!CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_DSCP))
+			return CMD_SUCCESS;
+		UNSET_FLAG(pbrms->filter_bm, PBR_FILTER_DSCP);
+		pbrms->dsfield &= ~PBR_DSFIELD_DSCP;
+		goto check;
 	}
-	dscpname[strlen(dscp)] = '\0';
 
-	if (isANumber) {
-		/* dscp passed is a regular number */
-		long dscpAsNum = strtol(dscp, NULL, 0);
+	unsigned long ul_dscp;
+	char *pend;
+	uint8_t raw_dscp;
 
-		if (dscpAsNum > PBR_DSFIELD_DSCP >> 2) {
-			/* Refuse to install on overflow */
-			vty_out(vty, "dscp (%s) must be less than 64\n", dscp);
-			return CMD_WARNING_CONFIG_FAILED;
-		}
-		rawDscp = dscpAsNum;
-	} else {
-		/* check dscp if it is an enum like cs0 */
-		rawDscp = pbr_map_decode_dscp_enum(dscpname);
-		if (rawDscp > PBR_DSFIELD_DSCP) {
-			vty_out(vty, "Invalid dscp value: %s\n", dscpname);
-			return CMD_WARNING_CONFIG_FAILED;
-		}
+	assert(dscp);
+	ul_dscp = strtol(dscp, &pend, 0);
+	if (*pend)
+		raw_dscp = pbr_map_decode_dscp_enum(dscp);
+	else
+		raw_dscp = ul_dscp << 2;
+	if (raw_dscp > PBR_DSFIELD_DSCP) {
+		vty_out(vty, "Invalid dscp value: %s%s\n", dscp,
+			(pend ? "" : " (numeric value must be in range 0-63)"));
+		return CMD_WARNING_CONFIG_FAILED;
 	}
 
-	if (!no) {
-		if (((pbrms->dsfield & PBR_DSFIELD_DSCP) >> 2) == rawDscp)
-			return CMD_SUCCESS;
-
-		/* Set the DSCP bits of the DSField */
-		pbrms->dsfield =
-			(pbrms->dsfield & ~PBR_DSFIELD_DSCP) | (rawDscp << 2);
-		SET_FLAG(pbrms->filter_bm, PBR_FILTER_DSCP);
-	} else {
-		pbrms->dsfield &= ~PBR_DSFIELD_DSCP;
-		UNSET_FLAG(pbrms->filter_bm, PBR_FILTER_DSCP);
+	if (CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_DSCP) &&
+	    (((pbrms->dsfield & PBR_DSFIELD_DSCP) >> 2) == raw_dscp)) {
+		return CMD_SUCCESS;
 	}
 
-	pbr_map_check(pbrms, true);
+	/* Set the DSCP bits of the DSField */
+	pbrms->dsfield = (pbrms->dsfield & ~PBR_DSFIELD_DSCP) | (raw_dscp << 2);
+	SET_FLAG(pbrms->filter_bm, PBR_FILTER_DSCP);
 
+check:
+	pbr_map_check(pbrms, true);
 	return CMD_SUCCESS;
 }
 
 /* clang-format off */
 DEFPY  (pbr_map_match_ecn,
 	pbr_map_match_ecn_cmd,
-	"[no] match ecn (0-3)$ecn",
+	"[no] match ecn ![(0-3)$ecn]",
 	NO_STR
 	"Match the rest of the command\n"
 	"Match based on IP ECN field\n"
@@ -387,20 +472,25 @@ DEFPY  (pbr_map_match_ecn,
 	if (!pbrms)
 		return CMD_WARNING_CONFIG_FAILED;
 
-	if (!no) {
-		if ((pbrms->dsfield & PBR_DSFIELD_ECN) == ecn)
+	if (no) {
+		if (!CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_ECN))
 			return CMD_SUCCESS;
-
-		/* Set the ECN bits of the DSField */
-		pbrms->dsfield = (pbrms->dsfield & ~PBR_DSFIELD_ECN) | ecn;
-		SET_FLAG(pbrms->filter_bm, PBR_FILTER_ECN);
-	} else {
-		pbrms->dsfield &= ~PBR_DSFIELD_ECN;
 		UNSET_FLAG(pbrms->filter_bm, PBR_FILTER_ECN);
+		pbrms->dsfield &= ~PBR_DSFIELD_ECN;
+		goto check;
 	}
 
-	pbr_map_check(pbrms, true);
+	if (CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_ECN) &&
+	    ((pbrms->dsfield & PBR_DSFIELD_ECN) == ecn)) {
+		return CMD_SUCCESS;
+	}
 
+	/* Set the ECN bits of the DSField */
+	pbrms->dsfield = (pbrms->dsfield & ~PBR_DSFIELD_ECN) | ecn;
+	SET_FLAG(pbrms->filter_bm, PBR_FILTER_ECN);
+
+check:
+	pbr_map_check(pbrms, true);
 	return CMD_SUCCESS;
 }
 
@@ -409,7 +499,9 @@ DEFPY  (pbr_map_match_ecn,
  ***********************************************************************/
 
 /* clang-format off */
-DEFPY  (pbr_map_match_pcp, pbr_map_match_pcp_cmd, "[no] match pcp <(0-7)$pcp>",
+DEFPY  (pbr_map_match_pcp,
+	pbr_map_match_pcp_cmd,
+	"[no] match pcp ![(0-7)$pcp]",
 	NO_STR
 	"Match spec follows\n"
 	"Match based on 802.1p Priority Code Point (PCP) value\n"
@@ -418,16 +510,33 @@ DEFPY  (pbr_map_match_pcp, pbr_map_match_pcp_cmd, "[no] match pcp <(0-7)$pcp>",
 	/* clang-format on */
 	struct pbr_map_sequence *pbrms = VTY_GET_CONTEXT(pbr_map_sequence);
 
-	if (pbrms)
-		pbr_set_match_clause_for_pcp(pbrms, !no, pcp);
+	if (!pbrms)
+		return CMD_WARNING_CONFIG_FAILED;
 
+	if (no) {
+		if (!CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_PCP))
+			return CMD_SUCCESS;
+		UNSET_FLAG(pbrms->filter_bm, PBR_FILTER_PCP);
+		goto check;
+	}
+
+	if (CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_PCP) &&
+	    (pbrms->match_pcp == pcp)) {
+		return CMD_SUCCESS;
+	}
+
+	pbrms->match_pcp = pcp;
+	SET_FLAG(pbrms->filter_bm, PBR_FILTER_PCP);
+
+check:
+	pbr_map_check(pbrms, true);
 	return CMD_SUCCESS;
 }
 
 /* clang-format off */
 DEFPY  (pbr_map_match_vlan_id,
 	pbr_map_match_vlan_id_cmd,
-	"[no] match vlan <(1-4094)$vlan_id>",
+	"[no] match vlan ![(1-4094)$vlan_id]",
 	NO_STR
 	"Match spec follows\n"
 	"Match based on VLAN ID\n"
@@ -436,19 +545,31 @@ DEFPY  (pbr_map_match_vlan_id,
 	/* clang-format on */
 	struct pbr_map_sequence *pbrms = VTY_GET_CONTEXT(pbr_map_sequence);
 
-	if (pbrms) {
-		if (!no) {
-			pbr_set_match_clause_for_vlan(pbrms, vlan_id, 0);
-		} else {
-			/* if the user previously set a vlan_id value */
-			if (pbrms->match_vlan_id != 0) {
-				if (vlan_id == pbrms->match_vlan_id) {
-					pbr_set_match_clause_for_vlan(pbrms, 0,
-								      0);
-				}
-			}
-		}
+	if (!pbrms)
+		return CMD_WARNING_CONFIG_FAILED;
+
+	if (no) {
+		if (!CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_VLAN_ID))
+			return CMD_SUCCESS;
+		UNSET_FLAG(pbrms->filter_bm, PBR_FILTER_VLAN_ID);
+		goto check;
 	}
+
+	if (CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_VLAN_ID) &&
+	    (pbrms->match_vlan_id == vlan_id)) {
+		return CMD_SUCCESS;
+	}
+
+	/*
+	 * Maintaining previous behavior: setting a vlan_id match
+	 * automatically clears any vlan_flags matching.
+	 */
+	UNSET_FLAG(pbrms->filter_bm, PBR_FILTER_VLAN_FLAGS);
+	SET_FLAG(pbrms->filter_bm, PBR_FILTER_VLAN_ID);
+	pbrms->match_vlan_id = vlan_id;
+
+check:
+	pbr_map_check(pbrms, true);
 	return CMD_SUCCESS;
 }
 
@@ -465,26 +586,45 @@ DEFPY  (pbr_map_match_vlan_tag,
 {
 	/* clang-format on */
 	struct pbr_map_sequence *pbrms = VTY_GET_CONTEXT(pbr_map_sequence);
+	uint16_t vlan_flags;
 
 	if (!pbrms)
-		return CMD_WARNING;
-
-	if (!no) {
-		assert(tag_type);
-		if (strmatch(tag_type, "tagged")) {
-			pbr_set_match_clause_for_vlan(pbrms, 0,
-						      PBR_VLAN_FLAGS_TAGGED);
-		} else if (strmatch(tag_type, "untagged")) {
-			pbr_set_match_clause_for_vlan(pbrms, 0,
-						      PBR_VLAN_FLAGS_UNTAGGED);
-		} else if (strmatch(tag_type, "untagged-or-zero")) {
-			pbr_set_match_clause_for_vlan(pbrms, 0,
-						      PBR_VLAN_FLAGS_UNTAGGED_0);
-		}
-	} else {
-		pbr_set_match_clause_for_vlan(pbrms, 0, PBR_VLAN_FLAGS_NO_WILD);
+		return CMD_WARNING_CONFIG_FAILED;
+
+	if (no) {
+		if (!CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_VLAN_FLAGS))
+			return CMD_SUCCESS;
+		UNSET_FLAG(pbrms->filter_bm, PBR_FILTER_VLAN_FLAGS);
+		goto check;
 	}
 
+	assert(tag_type);
+	if (strmatch(tag_type, "tagged"))
+		vlan_flags = PBR_VLAN_FLAGS_TAGGED;
+	else if (strmatch(tag_type, "untagged"))
+		vlan_flags = PBR_VLAN_FLAGS_UNTAGGED;
+	else if (strmatch(tag_type, "untagged-or-zero"))
+		vlan_flags = PBR_VLAN_FLAGS_UNTAGGED_0;
+	else {
+		vty_out(vty, "unknown vlan flag\n");
+		return CMD_WARNING_CONFIG_FAILED;
+	}
+
+	if (CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_VLAN_FLAGS) &&
+	    (pbrms->match_vlan_flags == vlan_flags)) {
+		return CMD_SUCCESS;
+	}
+
+	/*
+	 * Maintaining previous behavior: setting a vlan_flags match
+	 * automatically clears any vlan_id matching.
+	 */
+	UNSET_FLAG(pbrms->filter_bm, PBR_FILTER_VLAN_ID);
+	SET_FLAG(pbrms->filter_bm, PBR_FILTER_VLAN_FLAGS);
+	pbrms->match_vlan_flags = vlan_flags;
+
+check:
+	pbr_map_check(pbrms, true);
 	return CMD_SUCCESS;
 }
 
@@ -495,7 +635,7 @@ DEFPY  (pbr_map_match_vlan_tag,
 /* clang-format off */
 DEFPY  (pbr_map_match_mark,
 	pbr_map_match_mark_cmd,
-	"[no] match mark (1-4294967295)$mark",
+	"[no] match mark ![(1-4294967295)$mark]",
 	NO_STR
 	"Match the rest of the command\n"
 	"Choose the mark value to use\n"
@@ -512,15 +652,22 @@ DEFPY  (pbr_map_match_mark,
 	return CMD_WARNING_CONFIG_FAILED;
 #endif
 
-	if (!no) {
-		if (pbrms->mark)
-			if (pbrms->mark == (uint32_t)mark)
-				return CMD_SUCCESS;
+	if (no) {
+		if (!CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_FWMARK))
+			return CMD_SUCCESS;
+		UNSET_FLAG(pbrms->filter_bm, PBR_FILTER_FWMARK);
+		goto check;
+	}
 
-		pbrms->mark = (uint32_t)mark;
-	} else
-		pbrms->mark = 0;
+	if (CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_FWMARK) &&
+	    (pbrms->mark == (uint32_t)mark)) {
+		return CMD_SUCCESS;
+	}
 
+	pbrms->mark = (uint32_t)mark;
+	SET_FLAG(pbrms->filter_bm, PBR_FILTER_FWMARK);
+
+check:
 	pbr_map_check(pbrms, true);
 
 	return CMD_SUCCESS;
@@ -533,7 +680,7 @@ DEFPY  (pbr_map_match_mark,
 /* clang-format off */
 DEFPY  (pbr_map_action_queue_id,
 	pbr_map_action_queue_id_cmd,
-	"[no] set queue-id <(1-65535)$queue_id>",
+	"[no] set queue-id ![(1-65535)$queue_id]",
 	NO_STR
 	"Set the rest of the command\n"
 	"Set based on egress port queue id\n"
@@ -545,13 +692,22 @@ DEFPY  (pbr_map_action_queue_id,
 	if (!pbrms)
 		return CMD_WARNING_CONFIG_FAILED;
 
-	if (!no)
-		pbrms->action_queue_id = queue_id;
-	else if ((uint32_t)queue_id == pbrms->action_queue_id)
-		pbrms->action_queue_id = PBR_MAP_UNDEFINED_QUEUE_ID;
+	if (no) {
+		if (!CHECK_FLAG(pbrms->action_bm, PBR_ACTION_QUEUE_ID))
+			return CMD_SUCCESS;
+		UNSET_FLAG(pbrms->action_bm, PBR_ACTION_QUEUE_ID);
+		goto check;
+	}
 
-	pbr_map_check(pbrms, true);
+	if (CHECK_FLAG(pbrms->action_bm, PBR_ACTION_QUEUE_ID) &&
+	    (pbrms->action_queue_id == (uint32_t)queue_id)) {
+		return CMD_SUCCESS;
+	}
+	pbrms->action_queue_id = (uint32_t)queue_id;
+	SET_FLAG(pbrms->action_bm, PBR_ACTION_QUEUE_ID);
 
+check:
+	pbr_map_check(pbrms, true);
 	return CMD_SUCCESS;
 }
 
@@ -563,7 +719,7 @@ DEFPY  (pbr_map_action_queue_id,
 /* clang-format off */
 DEFPY  (pbr_map_action_pcp,
 	pbr_map_action_pcp_cmd,
-	"[no] set pcp <(0-7)$pcp>",
+	"[no] set pcp ![(0-7)$pcp]",
 	NO_STR
 	"Set the rest of the command\n"
 	"Set based on 802.1p Priority Code Point (PCP) value\n"
@@ -575,20 +731,30 @@ DEFPY  (pbr_map_action_pcp,
 	if (!pbrms)
 		return CMD_WARNING_CONFIG_FAILED;
 
-	if (!no)
-		pbrms->action_pcp = pcp;
-	else if (pcp == pbrms->action_pcp)
-		pbrms->action_pcp = 0;
+	if (no) {
+		if (!CHECK_FLAG(pbrms->action_bm, PBR_ACTION_PCP))
+			return CMD_SUCCESS;
+		UNSET_FLAG(pbrms->action_bm, PBR_ACTION_PCP);
+		goto check;
+	}
 
-	pbr_map_check(pbrms, true);
+	if (CHECK_FLAG(pbrms->action_bm, PBR_ACTION_PCP) &&
+	    pbrms->action_pcp == pcp) {
+		return CMD_SUCCESS;
+	}
+
+	pbrms->action_pcp = pcp;
+	SET_FLAG(pbrms->action_bm, PBR_ACTION_PCP);
 
+check:
+	pbr_map_check(pbrms, true);
 	return CMD_SUCCESS;
 }
 
 /* clang-format off */
 DEFPY  (pbr_map_action_vlan_id,
 	pbr_map_action_vlan_id_cmd,
-	"[no] set vlan <(1-4094)$vlan_id>",
+	"[no] set vlan ![(1-4094)$vlan_id]",
 	NO_STR
 	"Set the rest of the command\n"
 	"Set action for VLAN tagging\n"
@@ -600,13 +766,27 @@ DEFPY  (pbr_map_action_vlan_id,
 	if (!pbrms)
 		return CMD_WARNING_CONFIG_FAILED;
 
-	if (!no)
-		pbrms->action_vlan_id = vlan_id;
-	else if (pbrms->action_vlan_id == vlan_id)
-		pbrms->action_vlan_id = 0;
+	if (no) {
+		if (!CHECK_FLAG(pbrms->action_bm, PBR_ACTION_VLAN_ID))
+			return CMD_SUCCESS;
+		UNSET_FLAG(pbrms->action_bm, PBR_ACTION_VLAN_ID);
+		goto check;
+	}
 
-	pbr_map_check(pbrms, true);
+	if (CHECK_FLAG(pbrms->action_bm, PBR_ACTION_VLAN_ID) &&
+	    (pbrms->action_vlan_id == vlan_id)) {
+		return CMD_SUCCESS;
+	}
+
+	/*
+	 * Setting a vlan_id action automatically clears any strip-inner action
+	 */
+	pbrms->action_vlan_id = vlan_id;
+	UNSET_FLAG(pbrms->action_bm, PBR_ACTION_VLAN_STRIP_INNER_ANY);
+	SET_FLAG(pbrms->action_bm, PBR_ACTION_VLAN_ID);
 
+check:
+	pbr_map_check(pbrms, true);
 	return CMD_SUCCESS;
 }
 
@@ -616,7 +796,7 @@ DEFPY  (pbr_map_action_strip_vlan,
 	"[no] strip vlan",
 	NO_STR
 	"Strip the vlan tags from frame\n"
-	"Strip any inner vlan tag \n")
+	"Strip any inner vlan tag\n")
 {
 	/* clang-format on */
 	struct pbr_map_sequence *pbrms = VTY_GET_CONTEXT(pbr_map_sequence);
@@ -624,13 +804,24 @@ DEFPY  (pbr_map_action_strip_vlan,
 	if (!pbrms)
 		return CMD_WARNING_CONFIG_FAILED;
 
-	if (!no)
-		pbrms->action_vlan_flags = PBR_MAP_STRIP_INNER_ANY;
-	else
-		pbrms->action_vlan_flags = 0;
+	if (no) {
+		if (!CHECK_FLAG(pbrms->action_bm,
+				PBR_ACTION_VLAN_STRIP_INNER_ANY))
+			return CMD_SUCCESS;
+		UNSET_FLAG(pbrms->action_bm, PBR_ACTION_VLAN_STRIP_INNER_ANY);
+		goto check;
+	}
+	if (CHECK_FLAG(pbrms->action_bm, PBR_ACTION_VLAN_STRIP_INNER_ANY))
+		return CMD_SUCCESS;
 
-	pbr_map_check(pbrms, true);
+	/*
+	 * Setting a strip-inner action automatically clears any vlan_id action
+	 */
+	UNSET_FLAG(pbrms->action_bm, PBR_ACTION_VLAN_ID);
+	SET_FLAG(pbrms->action_bm, PBR_ACTION_VLAN_STRIP_INNER_ANY);
 
+check:
+	pbr_map_check(pbrms, true);
 	return CMD_SUCCESS;
 }
 
@@ -668,6 +859,8 @@ static void pbrms_clear_set_config(struct pbr_map_sequence *pbrms)
 	pbrms_clear_set_nexthop_config(pbrms);
 
 	pbrms->nhs_installed = false;
+
+	pbrms->forwarding_type = PBR_FT_UNSPEC;
 }
 
 
@@ -904,10 +1097,11 @@ DEFPY(pbr_map_vrf, pbr_map_vrf_cmd,
 	/*
 	 * If an equivalent set vrf * exists, just return success.
 	 */
-	if (vrf_name && pbrms->vrf_lookup
-	    && strncmp(pbrms->vrf_name, vrf_name, sizeof(pbrms->vrf_name)) == 0)
+	if ((pbrms->forwarding_type == PBR_FT_SETVRF) &&
+	    strncmp(pbrms->vrf_name, vrf_name, sizeof(pbrms->vrf_name)) == 0)
 		return CMD_SUCCESS;
-	else if (!vrf_name && pbrms->vrf_unchanged) /* Unchanged already set */
+	else if (!vrf_name && (pbrms->forwarding_type == PBR_FT_VRF_UNCHANGED))
+		/* Unchanged already set */
 		return CMD_SUCCESS;
 
 	if (vrf_name && !pbr_vrf_lookup_by_name(vrf_name)) {
@@ -920,9 +1114,12 @@ DEFPY(pbr_map_vrf, pbr_map_vrf_cmd,
 
 	if (vrf_name) {
 		pbrms->vrf_lookup = true;
+		pbrms->forwarding_type = PBR_FT_SETVRF;
 		strlcpy(pbrms->vrf_name, vrf_name, sizeof(pbrms->vrf_name));
-	} else
+	} else {
+		pbrms->forwarding_type = PBR_FT_VRF_UNCHANGED;
 		pbrms->vrf_unchanged = true;
+	}
 
 	pbr_map_check(pbrms, true);
 
@@ -1066,56 +1263,77 @@ static void vty_show_pbrms(struct vty *vty,
 			pbrms->installed ? "yes" : "no",
 			pbrms->reason ? rbuf : "Valid");
 
-	if (pbrms->ip_proto) {
+	/* match clauses first */
+
+	if (CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_IP_PROTOCOL)) {
 		struct protoent *p;
 
 		p = getprotobynumber(pbrms->ip_proto);
 		vty_out(vty, "        IP Protocol Match: %s\n", p->p_name);
 	}
 
-	if (pbrms->src)
+	if (CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_SRC_IP))
 		vty_out(vty, "        SRC IP Match: %pFX\n", pbrms->src);
-	if (pbrms->dst)
+	if (CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_DST_IP))
 		vty_out(vty, "        DST IP Match: %pFX\n", pbrms->dst);
-	if (pbrms->src_prt)
+
+	if (CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_SRC_PORT))
 		vty_out(vty, "        SRC Port Match: %u\n", pbrms->src_prt);
-	if (pbrms->dst_prt)
+	if (CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_DST_PORT))
 		vty_out(vty, "        DST Port Match: %u\n", pbrms->dst_prt);
 
-	if (pbrms->dsfield & PBR_DSFIELD_DSCP)
+	if (CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_DSCP))
 		vty_out(vty, "        DSCP Match: %u\n",
 			(pbrms->dsfield & PBR_DSFIELD_DSCP) >> 2);
-	if (pbrms->dsfield & PBR_DSFIELD_ECN)
+	if (CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_ECN))
 		vty_out(vty, "        ECN Match: %u\n",
 			pbrms->dsfield & PBR_DSFIELD_ECN);
-	if (pbrms->mark)
+
+	if (CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_FWMARK))
 		vty_out(vty, "        MARK Match: %u\n", pbrms->mark);
 	if (CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_PCP))
 		vty_out(vty, "        PCP Match: %d\n", pbrms->match_pcp);
 
-	if (pbrms->match_vlan_id != 0)
+	if (CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_VLAN_ID))
 		vty_out(vty, "        Match VLAN ID: %u\n",
 			pbrms->match_vlan_id);
-	if (pbrms->match_vlan_flags == PBR_VLAN_FLAGS_TAGGED)
-		vty_out(vty, "        Match VLAN tagged frames\n");
-	if (pbrms->match_vlan_flags == PBR_VLAN_FLAGS_UNTAGGED)
-		vty_out(vty, "        Match VLAN untagged frames\n");
-	if (pbrms->match_vlan_flags == PBR_VLAN_FLAGS_UNTAGGED_0)
-		vty_out(vty, "        Match VLAN untagged or ID 0\n");
-
-	if (pbrms->action_queue_id != PBR_MAP_UNDEFINED_QUEUE_ID)
+	if (CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_VLAN_FLAGS)) {
+		if (pbrms->match_vlan_flags == PBR_VLAN_FLAGS_TAGGED)
+			vty_out(vty, "        Match VLAN tagged frames\n");
+		if (pbrms->match_vlan_flags == PBR_VLAN_FLAGS_UNTAGGED)
+			vty_out(vty, "        Match VLAN untagged frames\n");
+		if (pbrms->match_vlan_flags == PBR_VLAN_FLAGS_UNTAGGED_0)
+			vty_out(vty, "        Match VLAN untagged or ID 0\n");
+	}
+
+	/* set actions */
+
+
+	if (CHECK_FLAG(pbrms->action_bm, PBR_ACTION_QUEUE_ID))
 		vty_out(vty, "        Set Queue ID: %u\n",
 			pbrms->action_queue_id);
 
-	if (pbrms->action_vlan_id != 0)
+	if (CHECK_FLAG(pbrms->action_bm, PBR_ACTION_VLAN_ID))
 		vty_out(vty, "        Set VLAN ID %u\n", pbrms->action_vlan_id);
-	if (pbrms->action_vlan_flags == PBR_MAP_STRIP_INNER_ANY)
+	if (CHECK_FLAG(pbrms->action_bm, PBR_ACTION_VLAN_STRIP_INNER_ANY))
 		vty_out(vty, "        Strip VLAN ID\n");
-	if (pbrms->action_pcp)
+	if (CHECK_FLAG(pbrms->action_bm, PBR_ACTION_PCP))
 		vty_out(vty, "        Set PCP %u\n", pbrms->action_pcp);
 
 
-	if (pbrms->nhgrp_name) {
+	switch (pbrms->forwarding_type) {
+	case PBR_FT_UNSPEC:
+		vty_out(vty, "        Nexthop-Group: Unknown Installed: no\n");
+		break;
+	case PBR_FT_VRF_UNCHANGED:
+		vty_out(vty, "        VRF Unchanged (use interface vrf)\n");
+		break;
+	case PBR_FT_SETVRF:
+		assert(pbrms->vrf_name);
+		vty_out(vty, "        VRF Lookup: %s\n", pbrms->vrf_name);
+		break;
+	case PBR_FT_NEXTHOP_GROUP:
+		assert(pbrms->nhgrp_name);
 		vty_out(vty, "        Nexthop-Group: %s\n", pbrms->nhgrp_name);
 
 		if (detail)
@@ -1129,8 +1347,9 @@ static void vty_show_pbrms(struct vty *vty,
 				pbr_nht_get_installed(pbrms->nhgrp_name) ? "yes"
 									 : "no",
 				pbr_nht_get_table(pbrms->nhgrp_name));
-
-	} else if (pbrms->nhg) {
+		break;
+	case PBR_FT_NEXTHOP_SINGLE:
+		assert(pbrms->internal_nhg_name);
 		vty_out(vty, "        ");
 		pbrms_nexthop_group_write_individual_nexthop(vty, pbrms);
 		if (detail)
@@ -1145,13 +1364,7 @@ static void vty_show_pbrms(struct vty *vty,
 					? "yes"
 					: "no",
 				pbr_nht_get_table(pbrms->internal_nhg_name));
-
-	} else if (pbrms->vrf_unchanged) {
-		vty_out(vty, "        VRF Unchanged (use interface vrf)\n");
-	} else if (pbrms->vrf_lookup) {
-		vty_out(vty, "        VRF Lookup: %s\n", pbrms->vrf_name);
-	} else {
-		vty_out(vty, "        Nexthop-Group: Unknown Installed: no\n");
+		break;
 	}
 }
 
@@ -1178,7 +1391,18 @@ static void vty_json_pbrms(json_object *j, struct vty *vty,
 	json_object_string_add(jpbrm, "installedReason",
 			       pbrms->reason ? rbuf : "Valid");
 
-	if (nhg_name) {
+	switch (pbrms->forwarding_type) {
+	case PBR_FT_UNSPEC:
+		break;
+	case PBR_FT_VRF_UNCHANGED:
+		break;
+	case PBR_FT_SETVRF:
+		assert(pbrms->vrf_name);
+		json_object_string_add(jpbrm, "vrfName", pbrms->vrf_name);
+		break;
+	case PBR_FT_NEXTHOP_GROUP:
+	case PBR_FT_NEXTHOP_SINGLE:
+		assert(nhg_name);
 		nexthop_group = json_object_new_object();
 
 		json_object_int_add(nexthop_group, "tableId",
@@ -1190,24 +1414,74 @@ static void vty_json_pbrms(json_object *j, struct vty *vty,
 				    pbrms->nhs_installed);
 
 		json_object_object_add(jpbrm, "nexthopGroup", nexthop_group);
+		break;
 	}
 
-	if (pbrms->vrf_lookup)
-		json_object_string_add(jpbrm, "vrfName", pbrms->vrf_name);
 
-	if (pbrms->src)
+	/*
+	 * Match clauses
+	 */
+
+	/* IP Header */
+	if (CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_IP_PROTOCOL))
+		json_object_int_add(jpbrm, "matchIpProtocol", pbrms->ip_proto);
+	if (CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_SRC_IP))
 		json_object_string_addf(jpbrm, "matchSrc", "%pFX", pbrms->src);
-	if (pbrms->dst)
+	if (CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_DST_IP))
 		json_object_string_addf(jpbrm, "matchDst", "%pFX", pbrms->dst);
-	if (pbrms->mark)
-		json_object_int_add(jpbrm, "matchMark", pbrms->mark);
-	if (pbrms->dsfield & PBR_DSFIELD_DSCP)
+	if (CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_SRC_PORT))
+		json_object_int_add(jpbrm, "matchSrcPort", pbrms->src_prt);
+	if (CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_DST_PORT))
+		json_object_int_add(jpbrm, "matchDstPort", pbrms->dst_prt);
+
+	if (CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_DSCP))
 		json_object_int_add(jpbrm, "matchDscp",
 				    (pbrms->dsfield & PBR_DSFIELD_DSCP) >> 2);
-	if (pbrms->dsfield & PBR_DSFIELD_ECN)
+	if (CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_ECN))
 		json_object_int_add(jpbrm, "matchEcn",
 				    pbrms->dsfield & PBR_DSFIELD_ECN);
 
+	/* L2 headers */
+	if (CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_PCP))
+		json_object_int_add(jpbrm, "matchPcp", pbrms->match_pcp);
+	if (CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_VLAN_ID))
+		json_object_int_add(jpbrm, "matchVlanId", pbrms->match_vlan_id);
+	if (CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_VLAN_FLAGS)) {
+		const char *p = "?";
+
+		if (pbrms->match_vlan_flags == PBR_VLAN_FLAGS_TAGGED)
+			p = "tagged";
+		if (pbrms->match_vlan_flags == PBR_VLAN_FLAGS_UNTAGGED)
+			p = "untagged";
+		if (pbrms->match_vlan_flags == PBR_VLAN_FLAGS_UNTAGGED_0)
+			p = "untagged-or-0";
+
+		json_object_string_addf(jpbrm, "matchVlanFlags", "%s", p);
+	}
+
+	/* meta */
+	if (CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_FWMARK))
+		json_object_int_add(jpbrm, "matchMark", pbrms->mark);
+
+	/*
+	 * action clauses
+	 */
+
+
+	/* L2 header fields */
+	if (CHECK_FLAG(pbrms->action_bm, PBR_ACTION_VLAN_STRIP_INNER_ANY))
+		json_object_boolean_true_add(jpbrm, "actionVlanStripInnerAny");
+	if (CHECK_FLAG(pbrms->action_bm, PBR_ACTION_VLAN_ID))
+		json_object_int_add(jpbrm, "actionSetVlanId",
+				    pbrms->action_vlan_id);
+	if (CHECK_FLAG(pbrms->action_bm, PBR_ACTION_PCP))
+		json_object_int_add(jpbrm, "actionSetPcp", pbrms->action_pcp);
+
+	/* meta */
+	if (CHECK_FLAG(pbrms->action_bm, PBR_ACTION_QUEUE_ID))
+		json_object_int_add(jpbrm, "actionSetQueueId",
+				    pbrms->action_queue_id);
+
 	json_object_array_add(j, jpbrm);
 }
 
@@ -1473,71 +1747,86 @@ static int pbr_vty_map_config_write_sequence(struct vty *vty,
 {
 	vty_out(vty, "pbr-map %s seq %u\n", pbrm->name, pbrms->seqno);
 
-	if (pbrms->src)
+	if (CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_IP_PROTOCOL)) {
+		struct protoent *p;
+
+		p = getprotobynumber(pbrms->ip_proto);
+		vty_out(vty, " match ip-protocol %s\n", p->p_name);
+	}
+
+	if (CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_SRC_IP))
 		vty_out(vty, " match src-ip %pFX\n", pbrms->src);
 
-	if (pbrms->dst)
+	if (CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_DST_IP))
 		vty_out(vty, " match dst-ip %pFX\n", pbrms->dst);
 
-	if (pbrms->src_prt)
+	if (CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_SRC_PORT))
 		vty_out(vty, " match src-port %u\n", pbrms->src_prt);
-	if (pbrms->dst_prt)
+	if (CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_DST_PORT))
 		vty_out(vty, " match dst-port %u\n", pbrms->dst_prt);
 
-	if (pbrms->ip_proto) {
-		struct protoent *p;
-
-		p = getprotobynumber(pbrms->ip_proto);
-		vty_out(vty, " match ip-protocol %s\n", p->p_name);
-	}
-
-	if (pbrms->dsfield & PBR_DSFIELD_DSCP)
+	if (CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_DSCP))
 		vty_out(vty, " match dscp %u\n",
 			(pbrms->dsfield & PBR_DSFIELD_DSCP) >> 2);
 
-	if (pbrms->dsfield & PBR_DSFIELD_ECN)
+	if (CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_ECN))
 		vty_out(vty, " match ecn %u\n",
 			pbrms->dsfield & PBR_DSFIELD_ECN);
 
-	if (pbrms->mark)
-		vty_out(vty, " match mark %u\n", pbrms->mark);
 	if (CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_PCP))
 		vty_out(vty, " match pcp %d\n", pbrms->match_pcp);
 
-	if ((pbrms->match_vlan_id) &&
-	    (pbrms->match_vlan_flags == PBR_VLAN_FLAGS_NO_WILD))
+	/* L2 headers */
+	if (CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_VLAN_ID))
 		vty_out(vty, " match vlan %u\n", pbrms->match_vlan_id);
-	if (pbrms->match_vlan_flags == PBR_VLAN_FLAGS_TAGGED)
-		vty_out(vty, " match vlan tagged\n");
-	if (pbrms->match_vlan_flags == PBR_VLAN_FLAGS_UNTAGGED)
-		vty_out(vty, " match vlan untagged\n");
-	if (pbrms->match_vlan_flags == PBR_VLAN_FLAGS_UNTAGGED_0)
-		vty_out(vty, " match vlan untagged-or-zero\n");
-
-	if (pbrms->action_queue_id != PBR_MAP_UNDEFINED_QUEUE_ID)
-		vty_out(vty, " set queue-id %d\n", pbrms->action_queue_id);
+	if (CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_VLAN_FLAGS)) {
+		if (pbrms->match_vlan_flags == PBR_VLAN_FLAGS_TAGGED)
+			vty_out(vty, " match vlan tagged\n");
+		if (pbrms->match_vlan_flags == PBR_VLAN_FLAGS_UNTAGGED)
+			vty_out(vty, " match vlan untagged\n");
+		if (pbrms->match_vlan_flags == PBR_VLAN_FLAGS_UNTAGGED_0)
+			vty_out(vty, " match vlan untagged-or-zero\n");
+	}
 
-	if (pbrms->action_pcp)
-		vty_out(vty, " set pcp %d\n", pbrms->action_pcp);
+	/* meta */
+	if (CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_FWMARK))
+		vty_out(vty, " match mark %u\n", pbrms->mark);
 
-	if (pbrms->action_vlan_id)
-		vty_out(vty, " set vlan %u\n", pbrms->action_vlan_id);
+	/*
+	 * action clauses
+	 */
 
-	if (pbrms->action_vlan_flags == PBR_MAP_STRIP_INNER_ANY)
+	/* L2 header fields */
+	if (CHECK_FLAG(pbrms->action_bm, PBR_ACTION_VLAN_STRIP_INNER_ANY))
 		vty_out(vty, " strip vlan any\n");
+	if (CHECK_FLAG(pbrms->action_bm, PBR_ACTION_VLAN_ID))
+		vty_out(vty, " set vlan %u\n", pbrms->action_vlan_id);
+	if (CHECK_FLAG(pbrms->action_bm, PBR_ACTION_PCP))
+		vty_out(vty, " set pcp %d\n", pbrms->action_pcp);
 
-	if (pbrms->vrf_unchanged)
-		vty_out(vty, " set vrf unchanged\n");
+	/* meta */
+	if (CHECK_FLAG(pbrms->action_bm, PBR_ACTION_QUEUE_ID))
+		vty_out(vty, " set queue-id %d\n", pbrms->action_queue_id);
 
-	if (pbrms->vrf_lookup)
+	switch (pbrms->forwarding_type) {
+	case PBR_FT_UNSPEC:
+		break;
+	case PBR_FT_VRF_UNCHANGED:
+		vty_out(vty, " set vrf unchanged\n");
+		break;
+	case PBR_FT_SETVRF:
+		assert(pbrms->vrf_name);
 		vty_out(vty, " set vrf %s\n", pbrms->vrf_name);
-
-	if (pbrms->nhgrp_name)
+		break;
+	case PBR_FT_NEXTHOP_GROUP:
+		assert(pbrms->nhgrp_name);
 		vty_out(vty, " set nexthop-group %s\n", pbrms->nhgrp_name);
-
-	if (pbrms->nhg) {
+		break;
+	case PBR_FT_NEXTHOP_SINGLE:
+		assert(pbrms->nhg);
 		vty_out(vty, " set ");
 		pbrms_nexthop_group_write_individual_nexthop(vty, pbrms);
+		break;
 	}
 
 	vty_out(vty, "exit\n");
@@ -1605,6 +1894,7 @@ void pbr_vty_init(void)
 	install_element(CONFIG_NODE, &pbr_set_table_range_cmd);
 	install_element(CONFIG_NODE, &no_pbr_set_table_range_cmd);
 	install_element(INTERFACE_NODE, &pbr_policy_cmd);
+
 	install_element(PBRMAP_NODE, &pbr_map_match_ip_proto_cmd);
 	install_element(PBRMAP_NODE, &pbr_map_match_src_port_cmd);
 	install_element(PBRMAP_NODE, &pbr_map_match_dst_port_cmd);
diff --git a/pbrd/pbr_zebra.c b/pbrd/pbr_zebra.c
index adcf449cf..66148c630 100644
--- a/pbrd/pbr_zebra.c
+++ b/pbrd/pbr_zebra.c
@@ -564,44 +564,18 @@ static bool pbr_encode_pbr_map_sequence(struct stream *s,
 	r.filter.fwmark = pbrms->mark;
 	r.filter.ip_proto = pbrms->ip_proto;
 
-	/*
-	 * Fix up filter flags for now, since PBRD doesn't maintain
-	 * them yet (aside from PBR_FILTER_PCP)
-	 */
-	if (!is_default_prefix(&r.filter.src_ip))
-		SET_FLAG(r.filter.filter_bm, PBR_FILTER_SRC_IP);
-	if (!is_default_prefix(&r.filter.dst_ip))
-		SET_FLAG(r.filter.filter_bm, PBR_FILTER_DST_IP);
-	if (r.filter.src_port)
-		SET_FLAG(r.filter.filter_bm, PBR_FILTER_SRC_PORT);
-	if (r.filter.dst_port)
-		SET_FLAG(r.filter.filter_bm, PBR_FILTER_DST_PORT);
-	if (r.filter.vlan_id)
-		SET_FLAG(r.filter.filter_bm, PBR_FILTER_VLAN_ID);
-	if (r.filter.vlan_flags)
-		SET_FLAG(r.filter.filter_bm, PBR_FILTER_VLAN_FLAGS);
-	if (r.filter.dsfield)
-		SET_FLAG(r.filter.filter_bm, PBR_FILTER_DSFIELD);
-	if (r.filter.fwmark)
-		SET_FLAG(r.filter.filter_bm, PBR_FILTER_FWMARK);
-	if (r.filter.ip_proto)
-		SET_FLAG(r.filter.filter_bm, PBR_FILTER_IP_PROTOCOL);
+	r.filter.filter_bm = pbrms->filter_bm;
 
 	/* actions */
 
+	SET_FLAG(r.action.flags, PBR_ACTION_TABLE); /* always valid */
+
 	/*
 	 * PBR should maintain its own set of action flags that we
 	 * can copy here instead of trying to infer from magic values.
 	 */
-	SET_FLAG(r.action.flags, PBR_ACTION_TABLE); /* always valid */
-	if (pbrms->action_queue_id != PBR_MAP_UNDEFINED_QUEUE_ID)
-		SET_FLAG(r.action.flags, PBR_ACTION_QUEUE_ID);
-	if (pbrms->action_pcp != 0)
-		SET_FLAG(r.action.flags, PBR_ACTION_PCP);
-	if (pbrms->action_vlan_id != 0)
-		SET_FLAG(r.action.flags, PBR_ACTION_VLAN_ID);
-	if (pbrms->action_vlan_flags != 0)
-		SET_FLAG(r.action.flags, PBR_ACTION_VLAN_FLAGS);
+
+	r.action.flags = pbrms->action_bm;
 
 	/*
 	 * if the user does not use the command "set vrf name unchanged"
@@ -619,9 +593,9 @@ static bool pbr_encode_pbr_map_sequence(struct stream *s,
 	}
 
 	r.action.queue_id = pbrms->action_queue_id;
+
 	r.action.pcp = pbrms->action_pcp;
 	r.action.vlan_id = pbrms->action_vlan_id;
-	r.action.vlan_flags = pbrms->action_vlan_flags;
 
 	strlcpy(r.ifname, ifp->name, sizeof(r.ifname));