Re: CVS commit: src/sys/dev/pci

[Paul Goyette <paul%whooppee.com@localhost>]

On Fri, 28 Oct 2016, Paul Goyette wrote:

> Attached is a revised patch, which has a prototype/signature much more 
> similar to snprintf(), more appropriate variable types, and improved 
> comments/documentation.
>
> I'll plan on committing this sometime late next week...

One more revision.  This one updates the length parameter as well as the 
dest pointer, so the caller doesn't need to recalculate on each call.

I've also changed it to return an int value rather than void:

	x = 0	success
	x < 0	vsnprintf() failed (for userland code, errno will
		contain details)
	x > 0	success, however the output was truncated to avoid
		overflowing the output buffer.

This variant seems to work just fine, both in-kernel and userland.


+------------------+--------------------------+------------------------+
| Paul Goyette     | PGP Key fingerprint:     | E-mail addresses:      |
| (Retired)        | FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com   |
| Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org |
+------------------+--------------------------+------------------------+

Index: pci_subr.c
===================================================================
RCS file: /cvsroot/src/sys/dev/pci/pci_subr.c,v
retrieving revision 1.152
diff -u -p -r1.152 pci_subr.c
--- pci_subr.c	20 Oct 2016 04:11:02 -0000	1.152
+++ pci_subr.c	30 Oct 2016 03:00:18 -0000
@@ -54,6 +54,7 @@ __KERNEL_RCSID(0, "$NetBSD: pci_subr.c,v
 #include <sys/module.h>
 #else
 #include <pci.h>
+#include <stdarg.h>
 #include <stdbool.h>
 #include <stdio.h>
 #include <stdlib.h>
@@ -567,6 +568,45 @@ static const struct pci_class pci_class[
 
 DEV_VERBOSE_DEFINE(pci);
 
+/*
+ * Append a formatted string to dest without writing more than len
+ * characters (including the trailing NUL character).  dest and len
+ * are updated for use in subsequent calls to snappendf().
+ *
+ * Returns 0 on success, a negative value if vnsprintf() fails, or
+ * a positive value if the dest buffer would have overflowed.
+ */
+static int
+snappendf(char **, size_t *, const char * restrict, ...) __printflike(3,4);
+
+static int
+snappendf(char **dest, size_t *len, const char * restrict fmt, ...)
+{
+	va_list	ap;
+	int count;
+
+	va_start(ap, fmt);
+	count = vsnprintf(*dest, *len, fmt, ap);
+	va_end(ap);
+
+	/* Let vsnprintf() errors bubble up to caller */
+	if (count < 0 || *len == 0)
+		return count;
+
+	/* Handle overflow */
+	if ((size_t)count >= *len) {
+		*dest += *len - 1;
+		*len = 1;
+		return 1;
+	}
+
+	/* Update dest & len to point at trailing NUL */
+	*dest += count;
+	*len -= count;
+		
+	return 0;
+}
+
 void
 pci_devinfo(pcireg_t id_reg, pcireg_t class_reg, int showclass, char *cp,
     size_t l)
@@ -577,9 +617,6 @@ pci_devinfo(pcireg_t id_reg, pcireg_t cl
 	pci_revision_t revision;
 	char vendor[PCI_VENDORSTR_LEN], product[PCI_PRODUCTSTR_LEN];
 	const struct pci_class *classp, *subclassp, *interfacep;
-	char *ep;
-
-	ep = cp + l;
 
 	pciclass = PCI_CLASS(class_reg);
 	subclass = PCI_SUBCLASS(class_reg);
@@ -612,32 +649,31 @@ pci_devinfo(pcireg_t id_reg, pcireg_t cl
 		interfacep++;
 	}
 
-	cp += snprintf(cp, ep - cp, "%s %s", vendor, product);
+	(void)snappendf(&cp, &l, "%s %s", vendor, product);
 	if (showclass) {
-		cp += snprintf(cp, ep - cp, " (");
+		(void)snappendf(&cp, &l, " (");
 		if (classp->name == NULL)
-			cp += snprintf(cp, ep - cp,
-			    "class 0x%02x, subclass 0x%02x", pciclass, subclass);
+			(void)snappendf(&cp, &l,
+			    "class 0x%02x, subclass 0x%02x",
+			    pciclass, subclass);
 		else {
 			if (subclassp == NULL || subclassp->name == NULL)
-				cp += snprintf(cp, ep - cp,
+				(void)snappendf(&cp, &l,
 				    "%s, subclass 0x%02x",
 				    classp->name, subclass);
 			else
-				cp += snprintf(cp, ep - cp, "%s %s",
+				(void)snappendf(&cp, &l, "%s %s",
 				    subclassp->name, classp->name);
 		}
 		if ((interfacep == NULL) || (interfacep->name == NULL)) {
 			if (interface != 0)
-				cp += snprintf(cp, ep - cp,
-				    ", interface 0x%02x", interface);
+				(void)snappendf(&cp, &l, ", interface 0x%02x",
+				    interface);
 		} else if (strncmp(interfacep->name, "", 1) != 0)
-			cp += snprintf(cp, ep - cp, ", %s",
-			    interfacep->name);
+			(void)snappendf(&cp, &l, ", %s", interfacep->name);
 		if (revision != 0)
-			cp += snprintf(cp, ep - cp, ", revision 0x%02x",
-			    revision);
-		cp += snprintf(cp, ep - cp, ")");
+			(void)snappendf(&cp, &l, ", revision 0x%02x", revision);
+		(void)snappendf(&cp, &l, ")");
 	}
 }