vrrpd, yang: cleanup vrrp nb conversion

- Use correct units and conversions in model & code
- Fix incorrect CLI help string for V6 virtual addrs
- Fix nb get-entry callback for virtual router
- Fix a couple style nits
- Simplify some CLI code
- Remove unused code
- Remove unused YANG definitions
- Update sighup() to handle reloads
- Update interface level config writer to use NB callbacks
- Add simplified `no` forms for priority and advertisement-interval
  commands

Signed-off-by: Quentin Young <qlyoung@cumulusnetworks.com>

6 files changed, 92 insertions, 185 deletions

diff --git a/vrrpd/vrrp.c b/vrrpd/vrrp.c
index 77bdf79b9..46cd12d71 100644
--- a/vrrpd/vrrp.c
+++ b/vrrpd/vrrp.c
@@ -458,8 +458,6 @@ int vrrp_add_ipv6(struct vrrp_vrouter *vr, struct in6_addr v6)
 	return vrrp_add_ip(vr, &ip);
 }
 
-
-
 int vrrp_del_ip(struct vrrp_vrouter *vr, struct ipaddr *ip)
 {
 	struct listnode *ln, *nn;
@@ -2264,71 +2262,6 @@ void vrrp_if_address_del(struct interface *ifp)
 
 /* Other ------------------------------------------------------------------- */
 
-int vrrp_config_write_interface(struct vty *vty)
-{
-	struct list *vrs = hash_to_list(vrrp_vrouters_hash);
-	struct listnode *ln, *ipln;
-	struct vrrp_vrouter *vr;
-	int writes = 0;
-
-	for (ALL_LIST_ELEMENTS_RO(vrs, ln, vr)) {
-		vty_frame(vty, "interface %s\n", vr->ifp->name);
-		++writes;
-
-		vty_out(vty, " vrrp %" PRIu8 "%s\n", vr->vrid,
-			vr->version == 2 ? " version 2" : "");
-		++writes;
-
-		if (vr->shutdown != vd.shutdown && ++writes)
-			vty_out(vty, " %svrrp %" PRIu8 " shutdown\n",
-				vr->shutdown ? "" : "no ", vr->vrid);
-
-		if (vr->preempt_mode != vd.preempt_mode && ++writes)
-			vty_out(vty, " %svrrp %" PRIu8 " preempt\n",
-				vr->preempt_mode ? "" : "no ", vr->vrid);
-
-		if (vr->accept_mode != vd.accept_mode && ++writes)
-			vty_out(vty, " %svrrp %" PRIu8 " accept\n",
-				vr->accept_mode ? "" : "no ", vr->vrid);
-
-		if (vr->advertisement_interval != vd.advertisement_interval
-		    && ++writes)
-			vty_out(vty,
-				" vrrp %" PRIu8
-				" advertisement-interval %d\n",
-				vr->vrid, vr->advertisement_interval * CS2MS);
-
-		if (vr->priority != vd.priority && ++writes)
-			vty_out(vty, " vrrp %" PRIu8 " priority %" PRIu8 "\n",
-				vr->vrid, vr->priority);
-
-		struct ipaddr *ip;
-
-		for (ALL_LIST_ELEMENTS_RO(vr->v4->addrs, ipln, ip)) {
-			char ipbuf[INET6_ADDRSTRLEN];
-
-			ipaddr2str(ip, ipbuf, sizeof(ipbuf));
-			vty_out(vty, " vrrp %" PRIu8 " ip %s\n", vr->vrid,
-				ipbuf);
-			++writes;
-		}
-
-		for (ALL_LIST_ELEMENTS_RO(vr->v6->addrs, ipln, ip)) {
-			char ipbuf[INET6_ADDRSTRLEN];
-
-			ipaddr2str(ip, ipbuf, sizeof(ipbuf));
-			vty_out(vty, " vrrp %" PRIu8 " ipv6 %s\n", vr->vrid,
-				ipbuf);
-			++writes;
-		}
-		vty_endframe(vty, "!\n");
-	}
-
-	list_delete(&vrs);
-
-	return writes;
-}
-
 int vrrp_config_write_global(struct vty *vty)
 {
 	unsigned int writes = 0;
diff --git a/vrrpd/vrrp.h b/vrrpd/vrrp.h
index 7e3e493a2..502d7b82b 100644
--- a/vrrpd/vrrp.h
+++ b/vrrpd/vrrp.h
@@ -550,17 +550,6 @@ void vrrp_if_address_del(struct interface *ifp);
 /* Other ------------------------------------------------------------------- */
 
 /*
- * Write interface block-level configuration to vty.
- *
- * vty
- *    vty to write config to
- *
- * Returns:
- *    # of lines written
- */
-int vrrp_config_write_interface(struct vty *vty);
-
-/*
  * Write global level configuration to vty.
  *
  * vty
diff --git a/vrrpd/vrrp_main.c b/vrrpd/vrrp_main.c
index 7bb3f0540..95b3cfad8 100644
--- a/vrrpd/vrrp_main.c
+++ b/vrrpd/vrrp_main.c
@@ -33,6 +33,7 @@
 #include "lib/sigevent.h"
 #include "lib/thread.h"
 #include "lib/vrf.h"
+#include "lib/vty.h"
 
 #include "vrrp.h"
 #include "vrrp_debug.h"
@@ -64,10 +65,14 @@ struct option longopts[] = { {0} };
 /* Master of threads. */
 struct thread_master *master;
 
+static struct frr_daemon_info vrrpd_di;
+
 /* SIGHUP handler. */
 static void sighup(void)
 {
 	zlog_info("SIGHUP received");
+
+	vty_read_config(NULL, vrrpd_di.config_file, config_default);
 }
 
 /* SIGINT / SIGTERM handler. */
diff --git a/vrrpd/vrrp_northbound.c b/vrrpd/vrrp_northbound.c
index cf9ae2e65..feaea6c03 100644
--- a/vrrpd/vrrp_northbound.c
+++ b/vrrpd/vrrp_northbound.c
@@ -118,9 +118,7 @@ lib_interface_vrrp_vrrp_group_lookup_entry(const void *parent_list_entry,
 	uint32_t vrid = strtoul(keys->key[0], NULL, 10);
 	const struct interface *ifp = parent_list_entry;
 
-	vrrp_lookup(ifp, vrid);
-
-	return NULL;
+	return vrrp_lookup(ifp, vrid);
 }
 
 /*
@@ -148,6 +146,9 @@ lib_interface_vrrp_vrrp_group_version_modify(enum nb_event event,
 	return NB_OK;
 }
 
+/*
+ * Helper function for address list OP_MODIFY callbacks.
+ */
 static void vrrp_yang_add_del_virtual_address(const struct lyd_node *dnode,
 					      bool add)
 {
@@ -164,9 +165,6 @@ static void vrrp_yang_add_del_virtual_address(const struct lyd_node *dnode,
 	vrrp_check_start(vr);
 }
 
-
-//-----------
-
 /*
  * XPath:
  * /frr-interface:lib/interface/frr-vrrpd:vrrp/vrrp-group/v4/virtual-address
@@ -401,11 +399,8 @@ lib_interface_vrrp_vrrp_group_v6_source_address_get_elem(const char *xpath,
 {
 	const struct vrrp_vrouter *vr = list_entry;
 	struct yang_data *val = NULL;
-	struct ipaddr ip;
-
-	memset(&ip, 0x00, sizeof(ip));
 
-	if (memcmp(&vr->v6->src.ipaddr_v6, &ip.ipaddr_v6, sizeof(ip.ipaddr_v6)))
+	if (ipaddr_isset(&vr->v6->src))
 		val = yang_data_new_ip(xpath, &vr->v6->src);
 
 	return val;
diff --git a/vrrpd/vrrp_vty.c b/vrrpd/vrrp_vty.c
index 4ccccc43a..892c8dadd 100644
--- a/vrrpd/vrrp_vty.c
+++ b/vrrpd/vrrp_vty.c
@@ -27,6 +27,7 @@
 #include "lib/prefix.h"
 #include "lib/termtable.h"
 #include "lib/vty.h"
+#include "lib/vrf.h"
 
 #include "vrrp.h"
 #include "vrrp_debug.h"
@@ -41,20 +42,9 @@
 #define VRRP_VRID_STR "Virtual Router ID\n"
 #define VRRP_PRIORITY_STR "Virtual Router Priority\n"
 #define VRRP_ADVINT_STR "Virtual Router Advertisement Interval\n"
-#define VRRP_IP_STR "Virtual Router IPv4 address\n"
+#define VRRP_IP_STR "Virtual Router IP address\n"
 #define VRRP_VERSION_STR "VRRP protocol version\n"
 
-#define VROUTER_GET_VTY(_vty, _ifp, _vrid, _vr)                                \
-	do {                                                                   \
-		_vr = vrrp_lookup(_ifp, _vrid);                                \
-		if (!_vr) {                                                    \
-			vty_out(_vty,                                          \
-				"%% Please configure VRRP instance %u\n",      \
-				(unsigned int)_vrid);                          \
-			return CMD_WARNING_CONFIG_FAILED;                      \
-		}                                                              \
-	} while (0)
-
 #define VRRP_XPATH_ENTRY VRRP_XPATH "[virtual-router-id='%ld']"
 
 /* clang-format off */
@@ -71,7 +61,7 @@ DEFPY(vrrp_vrid,
       VRRP_VERSION_STR
       VRRP_VERSION_STR)
 {
-	char valbuf[8];
+	char valbuf[20];
 
 	snprintf(valbuf, sizeof(valbuf), "%ld", version ? version : vd.version);
 
@@ -89,19 +79,11 @@ void cli_show_vrrp(struct vty *vty, struct lyd_node *dnode, bool show_defaults)
 {
 	const char *vrid = yang_dnode_get_string(dnode, "./virtual-router-id");
 	const char *ver = yang_dnode_get_string(dnode, "./version");
-	const char *dver =
-		yang_get_default_string("%s/version", VRRP_XPATH_FULL);
-
-	char verstr[16] = {};
-
-	if (strmatch(dver, ver)) {
-		if (show_defaults)
-			snprintf(verstr, sizeof(verstr), "version %s", dver);
-	} else {
-		snprintf(verstr, sizeof(verstr), "version %s", ver);
-	}
 
-	vty_out(vty, " vrrp %s %s\n", vrid, verstr);
+	vty_out(vty, " vrrp %s", vrid);
+	if (show_defaults || !yang_dnode_is_default(dnode, "./version"))
+		vty_out(vty, " version %s", ver);
+	vty_out(vty, "\n");
 }
 
 /*
@@ -135,16 +117,30 @@ void cli_show_shutdown(struct vty *vty, struct lyd_node *dnode,
  */
 DEFPY(vrrp_priority,
       vrrp_priority_cmd,
-      "[no] vrrp (1-255)$vrid priority (1-254)",
-      NO_STR
+      "vrrp (1-255)$vrid priority (1-254)",
       VRRP_STR
       VRRP_VRID_STR
       VRRP_PRIORITY_STR
       "Priority value")
 {
-	const char *val = no ? NULL : priority_str;
+	nb_cli_enqueue_change(vty, "./priority", NB_OP_MODIFY, priority_str);
 
-	nb_cli_enqueue_change(vty, "./priority", NB_OP_MODIFY, val);
+	return nb_cli_apply_changes(vty, VRRP_XPATH_ENTRY, vrid);
+}
+
+/*
+ * XPath: /frr-interface:lib/interface/frr-vrrpd:vrrp/vrrp-group/priority
+ */
+DEFPY(no_vrrp_priority,
+      no_vrrp_priority_cmd,
+      "no vrrp (1-255)$vrid priority [(1-254)]",
+      NO_STR
+      VRRP_STR
+      VRRP_VRID_STR
+      VRRP_PRIORITY_STR
+      "Priority value")
+{
+	nb_cli_enqueue_change(vty, "./priority", NB_OP_MODIFY, NULL);
 
 	return nb_cli_apply_changes(vty, VRRP_XPATH_ENTRY, vrid);
 }
@@ -164,21 +160,33 @@ void cli_show_priority(struct vty *vty, struct lyd_node *dnode,
  */
 DEFPY(vrrp_advertisement_interval,
       vrrp_advertisement_interval_cmd,
-      "[no] vrrp (1-255)$vrid advertisement-interval (10-40950)",
-      NO_STR VRRP_STR VRRP_VRID_STR VRRP_ADVINT_STR
+      "vrrp (1-255)$vrid advertisement-interval (10-40950)",
+      VRRP_STR VRRP_VRID_STR VRRP_ADVINT_STR
       "Advertisement interval in milliseconds; must be multiple of 10")
 {
-	char valbuf[8];
-	const char *val;
+	char val[20];
 
 	/* all internal computations are in centiseconds */
 	advertisement_interval /= CS2MS;
-	snprintf(valbuf, sizeof(valbuf), "%ld", advertisement_interval);
+	snprintf(val, sizeof(val), "%ld", advertisement_interval);
+	nb_cli_enqueue_change(vty, "./advertisement-interval", NB_OP_MODIFY,
+			      val);
 
-	val = no ? NULL : valbuf;
+	return nb_cli_apply_changes(vty, VRRP_XPATH_ENTRY, vrid);
+}
 
+/*
+ * XPath:
+ * /frr-interface:lib/interface/frr-vrrpd:vrrp/vrrp-group/advertisement-interval
+ */
+DEFPY(no_vrrp_advertisement_interval,
+      no_vrrp_advertisement_interval_cmd,
+      "no vrrp (1-255)$vrid advertisement-interval [(10-40950)]",
+      NO_STR VRRP_STR VRRP_VRID_STR VRRP_ADVINT_STR
+      "Advertisement interval in milliseconds; must be multiple of 10")
+{
 	nb_cli_enqueue_change(vty, "./advertisement-interval", NB_OP_MODIFY,
-			      val);
+			      NULL);
 
 	return nb_cli_apply_changes(vty, VRRP_XPATH_ENTRY, vrid);
 }
@@ -187,9 +195,10 @@ void cli_show_advertisement_interval(struct vty *vty, struct lyd_node *dnode,
 				     bool show_defaults)
 {
 	const char *vrid = yang_dnode_get_string(dnode, "../virtual-router-id");
-	const char *advi = yang_dnode_get_string(dnode, NULL);
+	uint16_t advint = yang_dnode_get_uint16(dnode, NULL);
 
-	vty_out(vty, " vrrp %s advertisement-interval %s\n", vrid, advi);
+	vty_out(vty, " vrrp %s advertisement-interval %u\n", vrid,
+		advint * CS2MS);
 }
 
 /*
@@ -706,6 +715,35 @@ DEFUN_NOSH (show_debugging_vrrp,
 
 /* clang-format on */
 
+/*
+ * Write per interface VRRP config.
+ */
+static int vrrp_config_write_interface(struct vty *vty)
+{
+	struct vrf *vrf;
+	int write = 0;
+
+	RB_FOREACH (vrf, vrf_name_head, &vrfs_by_name) {
+		struct interface *ifp;
+
+		FOR_ALL_INTERFACES (vrf, ifp) {
+			struct lyd_node *dnode;
+
+			dnode = yang_dnode_get(
+				running_config->dnode,
+				"/frr-interface:lib/interface[name='%s'][vrf='%s']",
+				ifp->name, vrf->name);
+			if (dnode == NULL)
+				continue;
+
+			write = 1;
+			nb_cli_show_dnode_cmds(vty, dnode, false);
+		}
+	}
+
+	return write;
+}
+
 static struct cmd_node interface_node = {INTERFACE_NODE, "%s(config-if)# ", 1};
 static struct cmd_node debug_node = {DEBUG_NODE, "", 1};
 static struct cmd_node vrrp_node = {VRRP_NODE, "", 1};
@@ -727,7 +765,9 @@ void vrrp_vty_init(void)
 	install_element(INTERFACE_NODE, &vrrp_vrid_cmd);
 	install_element(INTERFACE_NODE, &vrrp_shutdown_cmd);
 	install_element(INTERFACE_NODE, &vrrp_priority_cmd);
+	install_element(INTERFACE_NODE, &no_vrrp_priority_cmd);
 	install_element(INTERFACE_NODE, &vrrp_advertisement_interval_cmd);
+	install_element(INTERFACE_NODE, &no_vrrp_advertisement_interval_cmd);
 	install_element(INTERFACE_NODE, &vrrp_ip_cmd);
 	install_element(INTERFACE_NODE, &vrrp_ip6_cmd);
 	install_element(INTERFACE_NODE, &vrrp_preempt_cmd);
diff --git a/yang/frr-vrrpd.yang b/yang/frr-vrrpd.yang
index a9a5bd474..3d3a4138f 100644
--- a/yang/frr-vrrpd.yang
+++ b/yang/frr-vrrpd.yang
@@ -28,63 +28,6 @@ module frr-vrrpd {
       "Initial revision.";
   }
 
-  grouping ip-vrrp-tracking-config {
-    description
-      "Configuration data for tracking interfaces
-       in a VRRP group";
-    leaf-list track-interface {
-      type frr-interface:interface-ref;
-      description
-        "Sets a list of one or more interfaces that should
-         be tracked for up/down events to dynamically change the
-         priority state of the VRRP group, and potentially
-         change the mastership if the tracked interface going
-         down lowers the priority sufficiently. Any of the tracked
-         interfaces going down will cause the priority to be lowered.
-         Some implementations may only support a single
-         tracked interface.";
-    }
-
-    leaf priority-decrement {
-      type uint8 {
-        range "0..254";
-      }
-      default "0";
-      description
-        "Set the value to subtract from priority when
-         the tracked interface goes down";
-    }
-  }
-
-  grouping ip-vrrp-tracking-state {
-    description
-      "Operational state data for tracking interfaces in a VRRP
-       group";
-  }
-
-  grouping ip-vrrp-tracking-top {
-    description
-      "Top-level grouping for VRRP interface tracking";
-    container interface-tracking {
-      description
-        "Top-level container for VRRP interface tracking";
-      container config {
-        description
-          "Configuration data for VRRP interface tracking";
-        uses ip-vrrp-tracking-config;
-      }
-
-      container state {
-        config false;
-        description
-          "Operational state data for VRRP interface tracking";
-        uses ip-vrrp-tracking-config;
-
-        uses ip-vrrp-tracking-state;
-      }
-    }
-  }
-
   grouping ip-vrrp-config {
     description
       "Configuration data for VRRP on IP interfaces";
@@ -213,16 +156,18 @@ module frr-vrrpd {
       type uint16 {
         range "0..4095";
       }
+      units "centiseconds";
       config false;
       description
-        "Advertisement interval contained in advertisements received from the Master, in milliseconds.";
+        "Advertisement interval contained in advertisements received from the Master.";
     }
 
     leaf skew-time {
       type uint16;
+      units "centiseconds";
       config false;
       description
-        "Time to skew Master_Down_Interval, in milliseconds.";
+        "Time to skew Master_Down_Interval.";
     }
 
     container counter {