zebra: giant zapi cleanup

lib/log.c

@@ -1092,41 +1092,50 @@ void zlog_hexdump(const void *mem, unsigned int len)
 	unsigned long i = 0;
 	unsigned int j = 0;
 	unsigned int columns = 8;
-	char buf[(len * 4) + ((len / 4) * 20) + 30];
+	/*
+	 * 19 bytes for 0xADDRESS:
+	 * 24 bytes for data; 2 chars plus a space per data byte
+	 *  1 byte for space
+	 *  8 bytes for ASCII representation
+	 *  1 byte for a newline
+	 * =====================
+	 * 53 bytes per 8 bytes of data
+	 *  1 byte for null term
+	 */
+	size_t bs = ((len / 8) + 1) * 53 + 1;
+	char buf[bs];
 	char *s = buf;
 
 	for (i = 0; i < len + ((len % columns) ? (columns - len % columns) : 0);
 	     i++) {
 		/* print offset */
 		if (i % columns == 0)
-			s += sprintf(s, "0x%016lx: ", (unsigned long)mem + i);
+			s += snprintf(s, bs - (s - buf),
+				      "0x%016lx: ", (unsigned long)mem + i);
 
 		/* print hex data */
 		if (i < len)
-			s += sprintf(s, "%02x ", 0xFF & ((const char *)mem)[i]);
+			s += snprintf(s, bs - (s - buf), "%02x ",
+				      0xFF & ((const char *)mem)[i]);
 
 		/* end of block, just aligning for ASCII dump */
 		else
-			s += sprintf(s, "   ");
+			s += snprintf(s, bs - (s - buf), "   ");
 
 		/* print ASCII dump */
 		if (i % columns == (columns - 1)) {
 			for (j = i - (columns - 1); j <= i; j++) {
-				if (j >= len) /* end of block, not really
-						 printing */
-					s += sprintf(s, " ");
-
-				else if (isprint((int)((const char *)mem)
-							 [j])) /* printable char
-								  */
-					s += sprintf(
-						s, "%c",
+				/* end of block not really printing */
+				if (j >= len)
+					s += snprintf(s, bs - (s - buf), " ");
+				else if (isprint((int)((const char *)mem)[j]))
+					s += snprintf(
+						s, bs - (s - buf), "%c",
 						0xFF & ((const char *)mem)[j]);
-
 				else /* other char */
-					s += sprintf(s, ".");
+					s += snprintf(s, bs - (s - buf), ".");
 			}
-			s += sprintf(s, "\n");
+			s += snprintf(s, bs - (s - buf), "\n");
 		}
 	}
 	zlog_debug("\n%s", buf);

lib/zclient.c

@@ -322,6 +322,18 @@ int zclient_read_header(struct stream *s, int sock, u_int16_t *size,
 	return 0;
 }
 
+bool zapi_parse_header(struct stream *zmsg, struct zmsghdr *hdr)
+{
+	STREAM_GETW(zmsg, hdr->length);
+	STREAM_GETC(zmsg, hdr->marker);
+	STREAM_GETC(zmsg, hdr->version);
+	STREAM_GETL(zmsg, hdr->vrf_id);
+	STREAM_GETW(zmsg, hdr->command);
+	return true;
+stream_failure:
+	return false;
+}
+
 /* Send simple Zebra message. */
 static int zebra_message_send(struct zclient *zclient, int command,
 			      vrf_id_t vrf_id)

lib/zclient.h

@@ -237,14 +237,13 @@ struct zclient {
  */
 #define ZAPI_MESSAGE_TABLEID  0x80
 
+#define ZSERV_VERSION 5
 /* Zserv protocol message header */
-struct zserv_header {
+struct zmsghdr {
 	uint16_t length;
-	uint8_t marker; /* corresponds to command field in old zserv
-			 * always set to 255 in new zserv.
-			 */
+	/* Always set to 255 in new zserv */
+	uint8_t marker;
 	uint8_t version;
-#define ZSERV_VERSION	5
 	vrf_id_t vrf_id;
 	uint16_t command;
 };
@@ -380,9 +379,11 @@ struct zclient_options {
 /* Prototypes of zebra client service functions. */
 extern struct zclient *zclient_new(struct thread_master *);
 
+/* clang-format off */
 #if CONFDATE > 20181101
 CPP_NOTICE("zclient_new_notify can take over or zclient_new now");
 #endif
+/* clang-format on */
 
 extern struct zclient_options zclient_options_default;
 
@@ -449,9 +450,58 @@ extern int zclient_send_message(struct zclient *);
 
 /* create header for command, length to be filled in by user later */
 extern void zclient_create_header(struct stream *, uint16_t, vrf_id_t);
+/*
+ * Read sizeof(struct zmsghdr) bytes from the provided socket and parse the
+ * received data into the specified fields. If this is successful, read the
+ * rest of the packet into the provided stream.
+ *
+ * s
+ *    The stream to read into
+ *
+ * sock
+ *    The socket to read from
+ *
+ * size
+ *    Parsed message size will be placed in the pointed-at integer
+ *
+ * marker
+ *    Parsed marker will be placed in the pointed-at byte
+ *
+ * version
+ *    Parsed version will be placed in the pointed-at byte
+ *
+ * vrf_id
+ *    Parsed VRF ID will be placed in the pointed-at vrf_id_t
+ *
+ * cmd
+ *    Parsed command number will be placed in the pointed-at integer
+ *
+ * Returns:
+ *    -1 if:
+ *    - insufficient data for header was read
+ *    - a version mismatch was detected
+ *    - a marker mismatch was detected
+ *    - header size field specified more data than could be read
+ */
 extern int zclient_read_header(struct stream *s, int sock, u_int16_t *size,
 			       u_char *marker, u_char *version,
 			       vrf_id_t *vrf_id, u_int16_t *cmd);
+/*
+ * Parse header from ZAPI message stream into struct zmsghdr.
+ * This function assumes the stream getp points at the first byte of the header.
+ * If the function is successful then the stream getp will point to the byte
+ * immediately after the last byte of the header.
+ *
+ * zmsg
+ *    The stream containing the header
+ *
+ * hdr
+ *    The header struct to parse into.
+ *
+ * Returns:
+ *    true if parsing succeeded, false otherwise
+ */
+extern bool zapi_parse_header(struct stream *zmsg, struct zmsghdr *hdr);
 
 extern void zclient_interface_set_master(struct zclient *client,
 					 struct interface *master,
@@ -468,10 +518,11 @@ extern struct interface *zebra_interface_vrf_update_read(struct stream *s,
 extern void zebra_interface_if_set_value(struct stream *, struct interface *);
 extern void zebra_router_id_update_read(struct stream *s, struct prefix *rid);
 
+/* clang-format off */
 #if CONFDATE > 20180823
-CPP_NOTICE(
-	"zapi_ipv4_route, zapi_ipv6_route, zapi_ipv4_route_ipv6_nexthop as well as the zapi_ipv4 and zapi_ipv6 data structures should be removed now");
+CPP_NOTICE("zapi_ipv4_route, zapi_ipv6_route, zapi_ipv4_route_ipv6_nexthop as well as the zapi_ipv4 and zapi_ipv6 data structures should be removed now");
 #endif
+/* clang-format on */
 
 extern int zapi_ipv4_route(u_char, struct zclient *, struct prefix_ipv4 *,
 			   struct zapi_ipv4 *) __attribute__((deprecated));

tests/Makefile.am

@@ -73,6 +73,7 @@ check_PROGRAMS = \
 	lib/test_timer_correctness \
 	lib/test_timer_performance \
 	lib/test_ttable \
+	lib/test_zlog \
 	lib/cli/test_cli \
 	lib/cli/test_commands \
 	$(TESTS_BGPD) \
@@ -115,9 +116,9 @@ lib_test_heavy_SOURCES = lib/test_heavy.c helpers/c/main.c
 lib_test_memory_SOURCES = lib/test_memory.c
 lib_test_nexthop_iter_SOURCES = lib/test_nexthop_iter.c helpers/c/prng.c
 lib_test_privs_SOURCES = lib/test_privs.c
+lib_test_ringbuf_SOURCES = lib/test_ringbuf.c
 lib_test_srcdest_table_SOURCES = lib/test_srcdest_table.c \
                                  helpers/c/prng.c
-lib_test_ringbuf_SOURCES = lib/test_ringbuf.c
 lib_test_segv_SOURCES = lib/test_segv.c
 lib_test_sig_SOURCES = lib/test_sig.c
 lib_test_stream_SOURCES = lib/test_stream.c
@@ -127,6 +128,7 @@ lib_test_timer_correctness_SOURCES = lib/test_timer_correctness.c \
 lib_test_timer_performance_SOURCES = lib/test_timer_performance.c \
                                      helpers/c/prng.c
 lib_test_ttable_SOURCES = lib/test_ttable.c
+lib_test_zlog_SOURCES = lib/test_zlog.c
 lib_test_zmq_SOURCES = lib/test_zmq.c
 lib_test_zmq_CFLAGS = $(AM_CFLAGS) $(ZEROMQ_CFLAGS)
 lib_cli_test_cli_SOURCES = lib/cli/test_cli.c lib/cli/common_cli.c
@@ -167,6 +169,7 @@ lib_test_table_LDADD = $(ALL_TESTS_LDADD) -lm
 lib_test_timer_correctness_LDADD = $(ALL_TESTS_LDADD)
 lib_test_timer_performance_LDADD = $(ALL_TESTS_LDADD)
 lib_test_ttable_LDADD = $(ALL_TESTS_LDADD)
+lib_test_zlog_LDADD = $(ALL_TESTS_LDADD)
 lib_test_zmq_LDADD = ../lib/libfrrzmq.la $(ALL_TESTS_LDADD) $(ZEROMQ_LIBS)
 lib_cli_test_cli_LDADD = $(ALL_TESTS_LDADD)
 lib_cli_test_commands_LDADD = $(ALL_TESTS_LDADD)
@@ -207,6 +210,7 @@ EXTRA_DIST = \
     lib/test_timer_correctness.py \
     lib/test_ttable.py \
     lib/test_ttable.refout \
+    lib/test_zlog.py \
     ospf6d/test_lsdb.py \
     ospf6d/test_lsdb.in \
     ospf6d/test_lsdb.refout \

tests/lib/test_zlog.c

@@ -0,0 +1,61 @@
+/*
+ * Zlog tests.
+ * Copyright (C) 2018  Cumulus Networks, Inc.
+ *                     Quentin Young
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the Free
+ * Software Foundation; either version 2 of the License, or (at your option)
+ * any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; see the file COPYING; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+#include <zebra.h>
+#include <memory.h>
+#include "log.h"
+
+/* maximum amount of data to hexdump */
+#define MAXDATA 16384
+
+/*
+ * Test hexdump functionality.
+ *
+ * At the moment, not crashing is considered success.
+ */
+static bool test_zlog_hexdump(void)
+{
+	unsigned int nl = 1;
+
+	do {
+		long d[nl];
+
+		for (unsigned int i = 0; i < nl; i++)
+			d[i] = random();
+		zlog_hexdump(d, nl * sizeof(long));
+	} while (++nl * sizeof(long) <= MAXDATA);
+
+	return true;
+}
+
+bool (*tests[])(void) = {
+	test_zlog_hexdump,
+};
+
+int main(int argc, char **argv)
+{
+	openzlog("testzlog", "NONE", 0, LOG_CONS | LOG_NDELAY | LOG_PID,
+		 LOG_ERR);
+	zlog_set_file("test_zlog.log", LOG_DEBUG);
+
+	for (unsigned int i = 0; i < array_size(tests); i++)
+		if (!tests[i]())
+			return 1;
+	return 0;
+}

tests/lib/test_zlog.py

@@ -0,0 +1,4 @@
+import frrtest
+
+class TestZlog(frrtest.TestMultiOut):
+    program = './test_zlog'

zebra/label_manager.c

@@ -50,6 +50,8 @@ DEFINE_MTYPE_STATIC(LBL_MGR, LM_CHUNK, "Label Manager Chunk");
  * it will be a proxy to relay messages to external label manager
  * This zclient thus is to connect to it
  */
+static struct stream *ibuf;
+static struct stream *obuf;
 static struct zclient *zclient;
 bool lm_is_external;
 
@@ -69,7 +71,7 @@ static int relay_response_back(struct zserv *zserv)
 	u_int16_t resp_cmd;
 
 	src = zclient->ibuf;
-	dst = zserv->obuf;
+	dst = obuf;
 
 	stream_reset(src);
 
@@ -87,7 +89,7 @@ static int relay_response_back(struct zserv *zserv)
 
 	/* send response back */
 	stream_copy(dst, src);
-	ret = writen(zserv->sock, dst->data, stream_get_endp(dst));
+	ret = writen(zserv->sock, src->data, stream_get_endp(src));
 	if (ret <= 0) {
 		zlog_err("%s: Error sending Label Manager response back: %s",
 			 __func__, strerror(errno));
@@ -116,10 +118,10 @@ static int lm_zclient_read(struct thread *t)
 
 static int reply_error(int cmd, struct zserv *zserv, vrf_id_t vrf_id)
 {
+	int ret;
 	struct stream *s;
 
-	s = zserv->obuf;
-	stream_reset(s);
+	s = stream_new(ZEBRA_MAX_PACKET_SIZ);
 
 	zclient_create_header(s, cmd, vrf_id);
 
@@ -129,7 +131,10 @@ static int reply_error(int cmd, struct zserv *zserv, vrf_id_t vrf_id)
 	/* Write packet size. */
 	stream_putw_at(s, 0, stream_get_endp(s));
 
-	return writen(zserv->sock, s->data, stream_get_endp(s));
+	ret = writen(zserv->sock, s->data, stream_get_endp(s));
+
+	stream_free(s);
+	return ret;
 }
 /**
  * Receive a request to get or release a label chunk and forward it to external
@@ -161,7 +166,7 @@ int zread_relay_label_manager_request(int cmd, struct zserv *zserv,
 		ret = relay_response_back(zserv);
 
 	/* Send request to external label manager */
-	src = zserv->ibuf;
+	src = ibuf;
 	dst = zclient->obuf;
 
 	stream_copy(dst, src);
@@ -247,6 +252,9 @@ void label_manager_init(char *lm_zserv_path)
 		lm_is_external = true;
 		lm_zclient_init(lm_zserv_path);
 	}
+
+	ibuf = stream_new(ZEBRA_MAX_PACKET_SIZ);
+	obuf = stream_new(ZEBRA_MAX_PACKET_SIZ);
 }
 
 /**
@@ -379,4 +387,6 @@ int release_daemon_chunks(u_char proto, u_short instance)
 void label_manager_close()
 {
 	list_delete_and_null(&lbl_mgr.lc_list);
+	stream_free(ibuf);
+	stream_free(obuf);
 }