On Wed, Jan 10, 2024 at 01:02:54PM +0100, Theo Buehler wrote:
> On Wed, Jan 10, 2024 at 12:05:54PM +0100, Claudio Jeker wrote:
> > Extract the code to print a roa or aspa entry from printconf and convert
> > it into log_roa() and log_aspa().
> > 
> > Now log_aspa() is a bit complex because the size of the buffer is not
> > known. This now mallocs a buffer large enough and then tries to reuse that
> > buffer. I hope I did not screw this up too badly.
> > 
> > A follow up diff will use these log functions in the rtr code.
> 
> By definition of bikeshedding, simpler diffs generate more of it. Feel
> free to ignore.
> 
> ok tb
> 
> > +	/* include enough space for header and trailer */
> > +	if ((uint64_t)aspa->num > (SIZE_MAX / 12 - 72))
> > +		goto fail;
> > +	needed = (uint64_t)aspa->num * 12 + 72;
> 
> The 12 is a bit strange. How about using sizeof(asbuf)? Then you don't
> need to cast twice.

The if () still needs the cast to make the compiler not bark since an
uint32_t can never trigger the error on a 64bit arch.
 
> > +	if (needed > len) {
> > +		free(buf);
> > +		len = needed;
> > +		if ((buf = malloc(len)) == NULL)
> 
> not really nicer, but maybe use realloc?

See below.
 
> > +			goto fail;
> > +	}
> > +
> > +	snprintf(buf, len, "customer-as %s%s  provider-as { ",
> 
> is the double space before provider-as intentional?

Nope, fixed.

> > + fail:
> > +	{
> > +		static char errbuf[256];
> 
> I'd be inclined to make this have function scope. Then you could
> unindent and without the double space unwrap the snprintf.

Also fixed that. 

-- 
:wq Claudio

Index: bgpd.h
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/bgpd.h,v
diff -u -p -r1.479 bgpd.h
--- bgpd.h	8 Jan 2024 15:08:34 -0000	1.479
+++ bgpd.h	10 Jan 2024 10:36:44 -0000
@@ -1541,6 +1541,8 @@ const char	*log_as(uint32_t);
 const char	*log_rd(uint64_t);
 const char	*log_ext_subtype(int, uint8_t);
 const char	*log_reason(const char *);
+const char	*log_roa(struct roa *);
+const char	*log_aspa(struct aspa_set *);
 const char	*log_rtr_error(enum rtr_error);
 const char	*log_policy(enum role);
 int		 aspath_snprint(char *, size_t, void *, uint16_t);
Index: printconf.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/printconf.c,v
diff -u -p -r1.168 printconf.c
--- printconf.c	16 Aug 2023 08:26:35 -0000	1.168
+++ printconf.c	10 Jan 2024 10:59:23 -0000
@@ -687,22 +687,13 @@ void
 print_roa(struct roa_tree *r)
 {
 	struct roa	*roa;
-	struct bgpd_addr addr;
 
 	if (RB_EMPTY(r))
 		return;
 
 	printf("roa-set {");
 	RB_FOREACH(roa, roa_tree, r) {
-		printf("\n\t");
-		addr.aid = roa->aid;
-		addr.v6 = roa->prefix.inet6;
-		printf("%s/%u", log_addr(&addr), roa->prefixlen);
-		if (roa->prefixlen != roa->maxlen)
-			printf(" maxlen %u", roa->maxlen);
-		printf(" source-as %u", roa->asnum);
-		if (roa->expires != 0)
-			printf(" expires %lld", (long long)roa->expires);
+		printf("\n\t%s", log_roa(roa));
 	}
 	printf("\n}\n\n");
 }
@@ -711,21 +702,13 @@ void
 print_aspa(struct aspa_tree *a)
 {
 	struct aspa_set	*aspa;
-	uint32_t i;
 
 	if (RB_EMPTY(a))
 		return;
 
 	printf("aspa-set {");
 	RB_FOREACH(aspa, aspa_tree, a) {
-		printf("\n\t");
-		printf("customer-as %s", log_as(aspa->as));
-		if (aspa->expires != 0)
-			printf(" expires %lld", (long long)aspa->expires);
-		printf(" provider-as { ");
-		for (i = 0; i < aspa->num; i++)
-			printf("%s ", log_as(aspa->tas[i]));
-		printf("}");
+		printf("\n\t%s", log_aspa(aspa));
 	}
 	printf("\n}\n\n");
 }
Index: util.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/util.c,v
diff -u -p -r1.77 util.c
--- util.c	17 Apr 2023 08:02:21 -0000	1.77
+++ util.c	10 Jan 2024 13:11:32 -0000
@@ -165,6 +165,76 @@ log_reason(const char *communication) {
 	return buf;
 }
 
+static const char *
+log_expires(time_t expires)
+{
+	static char buf[32];
+
+	buf[0] = '\0';
+	if (expires != 0)
+		snprintf(buf, sizeof(buf), " expires %lld", (long long)expires);
+	return buf;
+}
+
+const char *
+log_roa(struct roa *roa)
+{
+	static char buf[256];
+	struct bgpd_addr addr = { .aid = roa->aid, .v6 = roa->prefix.inet6 };
+	char maxbuf[32];
+
+	maxbuf[0] = '\0';
+	if (roa->prefixlen != roa->maxlen)
+		snprintf(maxbuf, sizeof(maxbuf), " maxlen %u", roa->maxlen);
+	snprintf(buf, sizeof(buf), "%s/%u%s source-as %u%s", log_addr(&addr),
+	    roa->prefixlen, maxbuf, roa->asnum, log_expires(roa->expires));
+	return buf;
+}
+
+const char *
+log_aspa(struct aspa_set *aspa)
+{
+	static char errbuf[256];
+	static char *buf;
+	static size_t len;
+	char asbuf[16];
+	size_t needed;
+	uint32_t i;
+
+	/* include enough space for header and trailer */
+	if ((uint64_t)aspa->num > (SIZE_MAX / sizeof(asbuf) - 72))
+		goto fail;
+	needed = aspa->num * sizeof(asbuf) + 72;
+	if (needed > len) {
+		char *nbuf;
+
+		if ((nbuf = realloc(buf, needed)) == NULL)
+			goto fail;
+		len = needed;
+		buf = nbuf;
+	}
+
+	snprintf(buf, len, "customer-as %s%s provider-as { ",
+	    log_as(aspa->as), log_expires(aspa->expires));
+
+	for (i = 0; i < aspa->num; i++) {
+		snprintf(asbuf, sizeof(asbuf), "%s ", log_as(aspa->tas[i]));
+		if (strlcat(buf, asbuf, len) >= len)
+			goto fail;
+	}
+	if (strlcat(buf, "}", len) >= len)
+		goto fail;
+	return buf;
+
+ fail:
+	free(buf);
+	buf = NULL;
+	len = 0;
+	snprintf(errbuf, sizeof(errbuf), "customer-as %s%s provider-as { ... }",
+	    log_as(aspa->as), log_expires(aspa->expires));
+	return errbuf;
+}
+
 const char *
 log_rtr_error(enum rtr_error err)
 {