[PATCH 0/4] New line reading implementation

[PATCH 0/4] New line reading implementation

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 886 bytes --]

As discussed on our call, the current line_read() implementation is
pretty ugly and has some definite bugs.  This series replaces it with
a new implementation which should be more solid and more readable.

The new implementation is larger than I'd really like, but it turns
out its fiddlier to handle the various edge cases than you might
think.

David Gibson (4):
  Add cleaner line-by-line reading primitives
  Parse resolv.conf with new lineread implementation
  Use new lineread implementation for procfs_scan_listen()
  Remove unused line_read()

 Makefile   |   8 ++--
 conf.c     |  22 +++++++----
 lineread.c | 108 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 lineread.h |  23 ++++++++++++
 util.c     |  64 +++----------------------------
 5 files changed, 155 insertions(+), 70 deletions(-)
 create mode 100644 lineread.c
 create mode 100644 lineread.h

-- 
2.36.1

[PATCH 1/4] Add cleaner line-by-line reading primitives

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 5969 bytes --]

Two places in passt need to read files line by line (one parsing
resolv.conf, the other parsing /proc/net/*.  They can't use fgets()
because in glibc that can allocate memory.  Instead they use an
implementation line_read() in util.c.  This has some problems:

 * It has two completely separate modes of operation, one buffering
   and one not, the relation between these and how they're activated
   is subtle and confusing
 * At least in non-buffered mode, it will mishandle an empty line,
   folding them onto the start of the next non-empty line
 * In non-buffered mode it will use lseek() which prevents using this
   on non-regular files (we don't need that at present, but it's a
   surprising limitation)
 * It has a lot of difficult to read pointer mangling

Add a new cleaner implementation of allocation-free line-by-line
reading in lineread.c.  This one already buffers, using a state
structure to keep track of what we need.  This is larger than I'd
like, but it turns out handling all the edge cases of line-by-line
reading in C is surprisingly hard.

This just adds the code, subsequent patches will change the existing
users of line_read() to the new implementation.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 Makefile   |   8 ++--
 lineread.c | 108 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 lineread.h |  23 ++++++++++++
 3 files changed, 135 insertions(+), 4 deletions(-)
 create mode 100644 lineread.c
 create mode 100644 lineread.h

diff --git a/Makefile b/Makefile
index b0dde68..d059efb 100644
--- a/Makefile
+++ b/Makefile
@@ -32,16 +32,16 @@ CFLAGS += -DRLIMIT_STACK_VAL=$(RLIMIT_STACK_VAL)
 CFLAGS += -DARCH=\"$(TARGET_ARCH)\"
 
 PASST_SRCS = arch.c arp.c checksum.c conf.c dhcp.c dhcpv6.c icmp.c igmp.c \
-	mld.c ndp.c netlink.c packet.c passt.c pasta.c pcap.c siphash.c \
-	tap.c tcp.c tcp_splice.c udp.c util.c
+	lineread.c mld.c ndp.c netlink.c packet.c passt.c pasta.c pcap.c \
+	siphash.c tap.c tcp.c tcp_splice.c udp.c util.c
 QRAP_SRCS = qrap.c
 SRCS = $(PASST_SRCS) $(QRAP_SRCS)
 
 MANPAGES = passt.1 pasta.1 qrap.1
 
 PASST_HEADERS = arch.h arp.h checksum.h conf.h dhcp.h dhcpv6.h icmp.h \
-	ndp.h netlink.h packet.h passt.h pasta.h pcap.h siphash.h \
-	tap.h tcp.h tcp_splice.h udp.h util.h
+	lineread.h ndp.h netlink.h packet.h passt.h pasta.h pcap.h \
+	siphash.h tap.h tcp.h tcp_splice.h udp.h util.h
 HEADERS = $(PASST_HEADERS)
 
 # On gcc 11.2, with -O2 and -flto, tcp_hash() and siphash_20b(), if inlined,
diff --git a/lineread.c b/lineread.c
new file mode 100644
index 0000000..3e263cf
--- /dev/null
+++ b/lineread.c
@@ -0,0 +1,108 @@
+// SPDX-License-Identifier: AGPL-3.0-or-later
+
+/* PASST - Plug A Simple Socket Transport
+ *  for qemu/UNIX domain socket mode
+ *
+ * PASTA - Pack A Subtle Tap Abstraction
+ *  for network namespace/tap device mode
+ *
+ * lineread.c - Allocation free line-by-line buffered file input
+ *
+ * Copyright Red Hat
+ * Author: David Gibson <david(a)gibson.dropbear.id.au>
+ */
+
+#include <stddef.h>
+#include <fcntl.h>
+#include <string.h>
+#include <stdbool.h>
+#include <assert.h>
+#include <unistd.h>
+
+#include "lineread.h"
+
+/**
+ * lineread_init() - Prepare for line by line file reading without allocation
+ * @lr:		Line reader state structure to initialize
+ * @fd:		File handle to read lines from
+ */
+void lineread_init(struct lineread *lr, int fd)
+{
+	lr->fd = fd;
+	lr->next_line = lr->count = 0;
+}
+
+static int peek_line(struct lineread *lr, bool eof)
+{
+	char *nl;
+
+	/* Sanity checks (which also document invariants) */
+	assert(lr->count >= 0);
+	assert(lr->next_line >= 0);
+	assert(lr->next_line + lr->count >= lr->next_line);
+	assert(lr->next_line + lr->count <= LINEREAD_BUFFER_SIZE);
+
+	nl = memchr(lr->buf + lr->next_line, '\n', lr->count);
+
+	if (nl) {
+		*nl = '\0';
+		return nl - lr->buf - lr->next_line + 1;
+	} else if (eof) {
+		lr->buf[lr->next_line + lr->count] = '\0';
+		/*
+		 * No trailing newline, so treat all remaining bytes
+		 * as the last line
+		 */
+		return lr->count;
+	} else {
+		return -1;
+	}
+}
+
+/**
+ * lineread_get() - Read a single line from file (no allocation)
+ * @lr:		Line reader state structure
+ * @line:	Place a pointer to the next line in this variable
+ *
+ * Return:	Length of line read on success, 0 on EOF, negative on error
+ */
+int lineread_get(struct lineread *lr, char **line)
+{
+	bool eof = false;
+	int line_len;
+
+	while ((line_len = peek_line(lr, eof)) < 0) {
+		int rc;
+
+		if ((lr->next_line + lr->count) == LINEREAD_BUFFER_SIZE) {
+			/* No space at end */
+			if (lr->next_line == 0) {
+				/*
+				 * Buffer is full, which means we've
+				 * hit a line too long for us to
+				 * process.  FIXME: report error
+				 * better
+				 */
+				return -1;
+			}
+			memmove(lr->buf, lr->buf + lr->next_line, lr->count);
+			lr->next_line = 0;
+		}
+		
+		/* Read more data into the end of buffer */
+		rc = read(lr->fd, lr->buf + lr->next_line + lr->count,
+			  LINEREAD_BUFFER_SIZE - lr->next_line - lr->count);
+		if (rc < 0) {
+			return rc;
+		} else if (rc == 0) {
+			eof = true;
+		} else {
+			lr->count += rc;
+		}
+	}
+
+	*line = lr->buf + lr->next_line;
+	lr->next_line += line_len;
+	lr->count -= line_len;
+	return line_len;
+}
diff --git a/lineread.h b/lineread.h
new file mode 100644
index 0000000..972dc51
--- /dev/null
+++ b/lineread.h
@@ -0,0 +1,23 @@
+/* SPDX-License-Identifier: AGPL-3.0-or-later
+ * Copyright Red Hat
+ * Author: David Gibson <david(a)gibson.dropbear.id.au>
+ */
+
+#ifndef LINEREAD_H
+#define LINEREAD_H
+
+#define LINEREAD_BUFFER_SIZE	8192
+
+struct lineread {
+	int fd;
+	int next_line; /* start of next unread line in buffer */
+	int count; /* number of unreturned bytes in buffer */
+
+	/* One extra byte for possible trailing \0 */
+	char buf[LINEREAD_BUFFER_SIZE+1];
+};
+
+void lineread_init(struct lineread *lr, int fd);
+int lineread_get(struct lineread *lr, char **line);
+
+#endif /* _LINEREAD_H */
-- 
@@ -0,0 +1,23 @@
+/* SPDX-License-Identifier: AGPL-3.0-or-later
+ * Copyright Red Hat
+ * Author: David Gibson <david(a)gibson.dropbear.id.au>
+ */
+
+#ifndef LINEREAD_H
+#define LINEREAD_H
+
+#define LINEREAD_BUFFER_SIZE	8192
+
+struct lineread {
+	int fd;
+	int next_line; /* start of next unread line in buffer */
+	int count; /* number of unreturned bytes in buffer */
+
+	/* One extra byte for possible trailing \0 */
+	char buf[LINEREAD_BUFFER_SIZE+1];
+};
+
+void lineread_init(struct lineread *lr, int fd);
+int lineread_get(struct lineread *lr, char **line);
+
+#endif /* _LINEREAD_H */
-- 
2.36.1

[PATCH 2/4] Parse resolv.conf with new lineread implementation

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2283 bytes --]

Switch the resolv.conf parsing in conf.c to use the new lineread
implementation.  This means that it can now handle a resolv.conf file which
contains blank lines.

There are quite a few other fragilities with the resolv.conf parsing, but
that's out of scope for this patch.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 conf.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/conf.c b/conf.c
index d615cf5..471aeae 100644
--- a/conf.c
+++ b/conf.c
@@ -38,6 +38,7 @@
 #include "udp.h"
 #include "tcp.h"
 #include "pasta.h"
+#include "lineread.h"
 
 /**
  * get_bound_ports() - Get maps of ports with bound sockets
@@ -320,7 +321,9 @@ static void get_dns(struct ctx *c)
 	struct in6_addr *dns6 = &c->dns6[0];
 	struct fqdn *s = c->dns_search;
 	uint32_t *dns4 = &c->dns4[0];
-	char buf[BUFSIZ], *p, *end;
+	struct lineread resolvconf;
+	int line_len;
+	char *line, *p, *end;
 
 	dns4_set = !c->v4  || !!*dns4;
 	dns6_set = !c->v6  || !IN6_IS_ADDR_UNSPECIFIED(dns6);
@@ -333,13 +336,14 @@ static void get_dns(struct ctx *c)
 	if ((fd = open("/etc/resolv.conf", O_RDONLY | O_CLOEXEC)) < 0)
 		goto out;
 
-	for (*buf = 0; line_read(buf, BUFSIZ, fd); *buf = 0) {
-		if (!dns_set && strstr(buf, "nameserver ") == buf) {
-			p = strrchr(buf, ' ');
+	lineread_init(&resolvconf, fd);
+	while ((line_len = lineread_get(&resolvconf, &line)) > 0) {
+		if (!dns_set && strstr(line, "nameserver ") == line) {
+			p = strrchr(line, ' ');
 			if (!p)
 				continue;
 
-			end = strpbrk(buf, "%\n");
+			end = strpbrk(line, "%\n");
 			if (end)
 				*end = 0;
 
@@ -356,13 +360,13 @@ static void get_dns(struct ctx *c)
 				dns6++;
 				memset(dns6, 0, sizeof(*dns6));
 			}
-		} else if (!dnss_set && strstr(buf, "search ") == buf &&
+		} else if (!dnss_set && strstr(line, "search ") == line &&
 			   s == c->dns_search) {
-			end = strpbrk(buf, "\n");
+			end = strpbrk(line, "\n");
 			if (end)
 				*end = 0;
 
-			if (!strtok(buf, " \t"))
+			if (!strtok(line, " \t"))
 				continue;
 
 			while (s - c->dns_search < ARRAY_SIZE(c->dns_search) - 1
@@ -374,6 +378,8 @@ static void get_dns(struct ctx *c)
 		}
 	}
 
+	if (line_len < 0)
+		warn("Error reading /etc/resolv.conf: %s", strerror(errno));
 	close(fd);
 
 out:
-- 
@@ -38,6 +38,7 @@
 #include "udp.h"
 #include "tcp.h"
 #include "pasta.h"
+#include "lineread.h"
 
 /**
  * get_bound_ports() - Get maps of ports with bound sockets
@@ -320,7 +321,9 @@ static void get_dns(struct ctx *c)
 	struct in6_addr *dns6 = &c->dns6[0];
 	struct fqdn *s = c->dns_search;
 	uint32_t *dns4 = &c->dns4[0];
-	char buf[BUFSIZ], *p, *end;
+	struct lineread resolvconf;
+	int line_len;
+	char *line, *p, *end;
 
 	dns4_set = !c->v4  || !!*dns4;
 	dns6_set = !c->v6  || !IN6_IS_ADDR_UNSPECIFIED(dns6);
@@ -333,13 +336,14 @@ static void get_dns(struct ctx *c)
 	if ((fd = open("/etc/resolv.conf", O_RDONLY | O_CLOEXEC)) < 0)
 		goto out;
 
-	for (*buf = 0; line_read(buf, BUFSIZ, fd); *buf = 0) {
-		if (!dns_set && strstr(buf, "nameserver ") == buf) {
-			p = strrchr(buf, ' ');
+	lineread_init(&resolvconf, fd);
+	while ((line_len = lineread_get(&resolvconf, &line)) > 0) {
+		if (!dns_set && strstr(line, "nameserver ") == line) {
+			p = strrchr(line, ' ');
 			if (!p)
 				continue;
 
-			end = strpbrk(buf, "%\n");
+			end = strpbrk(line, "%\n");
 			if (end)
 				*end = 0;
 
@@ -356,13 +360,13 @@ static void get_dns(struct ctx *c)
 				dns6++;
 				memset(dns6, 0, sizeof(*dns6));
 			}
-		} else if (!dnss_set && strstr(buf, "search ") == buf &&
+		} else if (!dnss_set && strstr(line, "search ") == line &&
 			   s == c->dns_search) {
-			end = strpbrk(buf, "\n");
+			end = strpbrk(line, "\n");
 			if (end)
 				*end = 0;
 
-			if (!strtok(buf, " \t"))
+			if (!strtok(line, " \t"))
 				continue;
 
 			while (s - c->dns_search < ARRAY_SIZE(c->dns_search) - 1
@@ -374,6 +378,8 @@ static void get_dns(struct ctx *c)
 		}
 	}
 
+	if (line_len < 0)
+		warn("Error reading /etc/resolv.conf: %s", strerror(errno));
 	close(fd);
 
 out:
-- 
2.36.1

[PATCH 3/4] Use new lineread implementation for procfs_scan_listen()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1410 bytes --]

Use the new more solid implementation of line by line reading for
procfs_scan_listen().

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 util.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/util.c b/util.c
index 7ffd9d1..83729d2 100644
--- a/util.c
+++ b/util.c
@@ -41,6 +41,7 @@
 #include "util.h"
 #include "passt.h"
 #include "packet.h"
+#include "lineread.h"
 
 /* For __openlog() and __setlogmask() wrappers, and passt_vsyslog() */
 static int	log_mask;
@@ -476,7 +477,8 @@ char *line_read(char *buf, size_t len, int fd)
 void procfs_scan_listen(struct ctx *c, uint8_t proto, int ip_version, int ns,
 			uint8_t *map, uint8_t *exclude)
 {
-	char line[BUFSIZ], *path;
+	char *path, *line;
+	struct lineread lr;
 	unsigned long port;
 	unsigned int state;
 	int *fd;
@@ -500,9 +502,9 @@ void procfs_scan_listen(struct ctx *c, uint8_t proto, int ip_version, int ns,
 	else if ((*fd = open(path, O_RDONLY | O_CLOEXEC)) < 0)
 		return;
 
-	*line = 0;
-	line_read(line, sizeof(line), *fd);
-	while (line_read(line, sizeof(line), *fd)) {
+	lineread_init(&lr, *fd);
+	lineread_get(&lr, &line); /* throw away header */
+	while (lineread_get(&lr, &line) > 0) {
 		/* NOLINTNEXTLINE(cert-err34-c): != 2 if conversion fails */
 		if (sscanf(line, "%*u: %*x:%lx %*x:%*x %x", &port, &state) != 2)
 			continue;
-- 
@@ -41,6 +41,7 @@
 #include "util.h"
 #include "passt.h"
 #include "packet.h"
+#include "lineread.h"
 
 /* For __openlog() and __setlogmask() wrappers, and passt_vsyslog() */
 static int	log_mask;
@@ -476,7 +477,8 @@ char *line_read(char *buf, size_t len, int fd)
 void procfs_scan_listen(struct ctx *c, uint8_t proto, int ip_version, int ns,
 			uint8_t *map, uint8_t *exclude)
 {
-	char line[BUFSIZ], *path;
+	char *path, *line;
+	struct lineread lr;
 	unsigned long port;
 	unsigned int state;
 	int *fd;
@@ -500,9 +502,9 @@ void procfs_scan_listen(struct ctx *c, uint8_t proto, int ip_version, int ns,
 	else if ((*fd = open(path, O_RDONLY | O_CLOEXEC)) < 0)
 		return;
 
-	*line = 0;
-	line_read(line, sizeof(line), *fd);
-	while (line_read(line, sizeof(line), *fd)) {
+	lineread_init(&lr, *fd);
+	lineread_get(&lr, &line); /* throw away header */
+	while (lineread_get(&lr, &line) > 0) {
 		/* NOLINTNEXTLINE(cert-err34-c): != 2 if conversion fails */
 		if (sscanf(line, "%*u: %*x:%lx %*x:%*x %x", &port, &state) != 2)
 			continue;
-- 
2.36.1

[PATCH 4/4] Remove unused line_read()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1656 bytes --]

The old, ugly implementation of line_read() is no longer used.  Remove it.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 util.c | 54 ------------------------------------------------------
 1 file changed, 54 deletions(-)

diff --git a/util.c b/util.c
index 83729d2..98946e4 100644
--- a/util.c
+++ b/util.c
@@ -409,60 +409,6 @@ int bitmap_isset(const uint8_t *map, int bit)
 	return !!(*word & BITMAP_BIT(bit));
 }
 
-/**
- * line_read() - Similar to fgets(), no heap usage, a file instead of a stream
- * @buf:	Read buffer: on non-empty string, use that instead of reading
- * @len:	Maximum line length
- * @fd:		File descriptor for reading
- *
- * Return: @buf if a line is found, NULL on EOF or error
- */
-char *line_read(char *buf, size_t len, int fd)
-{
-	int n, do_read = !*buf;
-	char *p;
-
-	if (!do_read) {
-		char *nl;
-
-		buf[len - 1] = 0;
-		if (!strlen(buf))
-			return NULL;
-
-		p = buf + strlen(buf) + 1;
-
-		while (*p == '\n' && strlen(p) && (size_t)(p - buf) < len)
-			p++;
-
-		if (!(nl = strchr(p, '\n')))
-			return NULL;
-		*nl = 0;
-
-		return memmove(buf, p, len - (p - buf));
-	}
-
-	n = read(fd, buf, --len);
-	if (n <= 0)
-		return NULL;
-
-	buf[len] = 0;
-
-	p = buf;
-	while (*p == '\n' && strlen(p) && (size_t)(p - buf) < len)
-		p++;
-
-	if (!(p = strchr(p, '\n')))
-		return buf;
-
-	*p = 0;
-	if (p == buf)
-		return buf;
-
-	lseek(fd, (p - buf) - n + 1, SEEK_CUR);
-
-	return buf;
-}
-
 /**
  * procfs_scan_listen() - Set bits for listening TCP or UDP sockets from procfs
  * @proto:	IPPROTO_TCP or IPPROTO_UDP
-- 
@@ -409,60 +409,6 @@ int bitmap_isset(const uint8_t *map, int bit)
 	return !!(*word & BITMAP_BIT(bit));
 }
 
-/**
- * line_read() - Similar to fgets(), no heap usage, a file instead of a stream
- * @buf:	Read buffer: on non-empty string, use that instead of reading
- * @len:	Maximum line length
- * @fd:		File descriptor for reading
- *
- * Return: @buf if a line is found, NULL on EOF or error
- */
-char *line_read(char *buf, size_t len, int fd)
-{
-	int n, do_read = !*buf;
-	char *p;
-
-	if (!do_read) {
-		char *nl;
-
-		buf[len - 1] = 0;
-		if (!strlen(buf))
-			return NULL;
-
-		p = buf + strlen(buf) + 1;
-
-		while (*p == '\n' && strlen(p) && (size_t)(p - buf) < len)
-			p++;
-
-		if (!(nl = strchr(p, '\n')))
-			return NULL;
-		*nl = 0;
-
-		return memmove(buf, p, len - (p - buf));
-	}
-
-	n = read(fd, buf, --len);
-	if (n <= 0)
-		return NULL;
-
-	buf[len] = 0;
-
-	p = buf;
-	while (*p == '\n' && strlen(p) && (size_t)(p - buf) < len)
-		p++;
-
-	if (!(p = strchr(p, '\n')))
-		return buf;
-
-	*p = 0;
-	if (p == buf)
-		return buf;
-
-	lseek(fd, (p - buf) - n + 1, SEEK_CUR);
-
-	return buf;
-}
-
 /**
  * procfs_scan_listen() - Set bits for listening TCP or UDP sockets from procfs
  * @proto:	IPPROTO_TCP or IPPROTO_UDP
-- 
2.36.1

Re: [PATCH 1/4] Add cleaner line-by-line reading primitives

[Stefano Brivio]

[-- Attachment #1: Type: text/plain, Size: 8005 bytes --]

On Fri, 17 Jun 2022 13:10:23 +1000
David Gibson <david(a)gibson.dropbear.id.au> wrote:

> Two places in passt need to read files line by line (one parsing
> resolv.conf, the other parsing /proc/net/*.  They can't use fgets()
> because in glibc that can allocate memory.  Instead they use an
> implementation line_read() in util.c.  This has some problems:
> 
>  * It has two completely separate modes of operation, one buffering
>    and one not, the relation between these and how they're activated
>    is subtle and confusing
>  * At least in non-buffered mode, it will mishandle an empty line,
>    folding them onto the start of the next non-empty line
>  * In non-buffered mode it will use lseek() which prevents using this
>    on non-regular files (we don't need that at present, but it's a
>    surprising limitation)
>  * It has a lot of difficult to read pointer mangling
> 
> Add a new cleaner implementation of allocation-free line-by-line
> reading in lineread.c.  This one already buffers, using a state
> structure to keep track of what we need.  This is larger than I'd
> like, but it turns out handling all the edge cases of line-by-line
> reading in C is surprisingly hard.

Still much simpler (albeit a bit more verbose) than the original
version, thanks. :)

> This just adds the code, subsequent patches will change the existing
> users of line_read() to the new implementation.
> 
> Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
> ---
>  Makefile   |   8 ++--
>  lineread.c | 108 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  lineread.h |  23 ++++++++++++
>  3 files changed, 135 insertions(+), 4 deletions(-)
>  create mode 100644 lineread.c
>  create mode 100644 lineread.h
> 
> diff --git a/Makefile b/Makefile
> index b0dde68..d059efb 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -32,16 +32,16 @@ CFLAGS += -DRLIMIT_STACK_VAL=$(RLIMIT_STACK_VAL)
>  CFLAGS += -DARCH=\"$(TARGET_ARCH)\"
>  
>  PASST_SRCS = arch.c arp.c checksum.c conf.c dhcp.c dhcpv6.c icmp.c igmp.c \
> -	mld.c ndp.c netlink.c packet.c passt.c pasta.c pcap.c siphash.c \
> -	tap.c tcp.c tcp_splice.c udp.c util.c
> +	lineread.c mld.c ndp.c netlink.c packet.c passt.c pasta.c pcap.c \
> +	siphash.c tap.c tcp.c tcp_splice.c udp.c util.c
>  QRAP_SRCS = qrap.c
>  SRCS = $(PASST_SRCS) $(QRAP_SRCS)
>  
>  MANPAGES = passt.1 pasta.1 qrap.1
>  
>  PASST_HEADERS = arch.h arp.h checksum.h conf.h dhcp.h dhcpv6.h icmp.h \
> -	ndp.h netlink.h packet.h passt.h pasta.h pcap.h siphash.h \
> -	tap.h tcp.h tcp_splice.h udp.h util.h
> +	lineread.h ndp.h netlink.h packet.h passt.h pasta.h pcap.h \
> +	siphash.h tap.h tcp.h tcp_splice.h udp.h util.h
>  HEADERS = $(PASST_HEADERS)
>  
>  # On gcc 11.2, with -O2 and -flto, tcp_hash() and siphash_20b(), if inlined,
> diff --git a/lineread.c b/lineread.c
> new file mode 100644
> index 0000000..3e263cf
> --- /dev/null
> +++ b/lineread.c
> @@ -0,0 +1,108 @@
> +// SPDX-License-Identifier: AGPL-3.0-or-later
> +
> +/* PASST - Plug A Simple Socket Transport
> + *  for qemu/UNIX domain socket mode
> + *
> + * PASTA - Pack A Subtle Tap Abstraction
> + *  for network namespace/tap device mode
> + *
> + * lineread.c - Allocation free line-by-line buffered file input
> + *
> + * Copyright Red Hat
> + * Author: David Gibson <david(a)gibson.dropbear.id.au>
> + */
> +
> +#include <stddef.h>
> +#include <fcntl.h>
> +#include <string.h>
> +#include <stdbool.h>
> +#include <assert.h>
> +#include <unistd.h>
> +
> +#include "lineread.h"
> +
> +/**
> + * lineread_init() - Prepare for line by line file reading without allocation
> + * @lr:		Line reader state structure to initialize
> + * @fd:		File handle to read lines from

I think by "handle" most people refer to "FILE" stream handles, I would
drop "handle" here or replace by "descriptor".

> + */
> +void lineread_init(struct lineread *lr, int fd)
> +{
> +	lr->fd = fd;
> +	lr->next_line = lr->count = 0;
> +}
> +
> +static int peek_line(struct lineread *lr, bool eof)

This lacks a description in kerneldoc style, I would add:

/**
 * peek_line() - Find and NULL-terminate next line in buffer
 * @lr:		Line reader state structure
 * @eof:	Caller indicates end-of-file was already found by read()
 *
 * Return: length of line in bytes, -1 if no line was found
 */

By the way, if you're wondering why you're introducing the first usage
of 'bool', I switched to C99 just recently. :)

> +{
> +	char *nl;
> +
> +	/* Sanity checks (which also document invariants) */
> +	assert(lr->count >= 0);
> +	assert(lr->next_line >= 0);
> +	assert(lr->next_line + lr->count >= lr->next_line);
> +	assert(lr->next_line + lr->count <= LINEREAD_BUFFER_SIZE);
> +
> +	nl = memchr(lr->buf + lr->next_line, '\n', lr->count);
> +
> +	if (nl) {
> +		*nl = '\0';
> +		return nl - lr->buf - lr->next_line + 1;

clang-tidy complains about the else-after-return here
(llvm-else-after-return, readability-else-after-return), and at a
second glance me too :) I would drop them if you're fine with it.

> +	} else if (eof) {
> +		lr->buf[lr->next_line + lr->count] = '\0';
> +		/*
> +		 * No trailing newline, so treat all remaining bytes
> +		 * as the last line
> +		 */
> +		return lr->count;
> +	} else {
> +		return -1;
> +	}
> +}
> +
> +/**
> + * lineread_get() - Read a single line from file (no allocation)
> + * @lr:		Line reader state structure
> + * @line:	Place a pointer to the next line in this variable
> + *
> + * Return:	Length of line read on success, 0 on EOF, negative on error
> + */
> +int lineread_get(struct lineread *lr, char **line)
> +{
> +	bool eof = false;
> +	int line_len;
> +
> +	while ((line_len = peek_line(lr, eof)) < 0) {
> +		int rc;
> +
> +		if ((lr->next_line + lr->count) == LINEREAD_BUFFER_SIZE) {
> +			/* No space at end */
> +			if (lr->next_line == 0) {
> +				/*

Nit: elsewhere, I used "net" kernel style comments, which are the same
as every other area of the Linux kernel except that there's no opening
newline:

				/* Buffer is full, which means we've

I would change it here and in the comment above.

> +				 * Buffer is full, which means we've
> +				 * hit a line too long for us to
> +				 * process.  FIXME: report error
> +				 * better
> +				 */
> +				return -1;
> +			}
> +			memmove(lr->buf, lr->buf + lr->next_line, lr->count);
> +			lr->next_line = 0;
> +		}
> +		

Stray tabs here, dropped.


> +		/* Read more data into the end of buffer */
> +		rc = read(lr->fd, lr->buf + lr->next_line + lr->count,
> +			  LINEREAD_BUFFER_SIZE - lr->next_line - lr->count);
> +		if (rc < 0) {
> +			return rc;
> +		} else if (rc == 0) {
> +			eof = true;
> +		} else {
> +			lr->count += rc;
> +		}
> +	}
> +
> +	*line = lr->buf + lr->next_line;
> +	lr->next_line += line_len;
> +	lr->count -= line_len;
> +	return line_len;
> +}
> diff --git a/lineread.h b/lineread.h
> new file mode 100644
> index 0000000..972dc51
> --- /dev/null
> +++ b/lineread.h
> @@ -0,0 +1,23 @@
> +/* SPDX-License-Identifier: AGPL-3.0-or-later
> + * Copyright Red Hat
> + * Author: David Gibson <david(a)gibson.dropbear.id.au>
> + */
> +
> +#ifndef LINEREAD_H
> +#define LINEREAD_H
> +
> +#define LINEREAD_BUFFER_SIZE	8192
> +

I would also stick to kerneldoc style comment here:

/**
 * struct lineread - Line reader state
 * @fd:		File descriptor lines are read from
 * @next_line:	...

> +struct lineread {
> +	int fd;
> +	int next_line; /* start of next unread line in buffer */
> +	int count; /* number of unreturned bytes in buffer */
> +
> +	/* One extra byte for possible trailing \0 */
> +	char buf[LINEREAD_BUFFER_SIZE+1];
> +};
> +
> +void lineread_init(struct lineread *lr, int fd);
> +int lineread_get(struct lineread *lr, char **line);
> +
> +#endif /* _LINEREAD_H */

I can change stuff on merge, let me know.

-- 
Stefano

Re: [PATCH 1/4] Add cleaner line-by-line reading primitives

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 9569 bytes --]

On Thu, Jun 23, 2022 at 11:31:43AM +0200, Stefano Brivio wrote:
> On Fri, 17 Jun 2022 13:10:23 +1000
> David Gibson <david(a)gibson.dropbear.id.au> wrote:
> 
> > Two places in passt need to read files line by line (one parsing
> > resolv.conf, the other parsing /proc/net/*.  They can't use fgets()
> > because in glibc that can allocate memory.  Instead they use an
> > implementation line_read() in util.c.  This has some problems:
> > 
> >  * It has two completely separate modes of operation, one buffering
> >    and one not, the relation between these and how they're activated
> >    is subtle and confusing
> >  * At least in non-buffered mode, it will mishandle an empty line,
> >    folding them onto the start of the next non-empty line
> >  * In non-buffered mode it will use lseek() which prevents using this
> >    on non-regular files (we don't need that at present, but it's a
> >    surprising limitation)
> >  * It has a lot of difficult to read pointer mangling
> > 
> > Add a new cleaner implementation of allocation-free line-by-line
> > reading in lineread.c.  This one already buffers, using a state
> > structure to keep track of what we need.  This is larger than I'd
> > like, but it turns out handling all the edge cases of line-by-line
> > reading in C is surprisingly hard.
> 
> Still much simpler (albeit a bit more verbose) than the original
> version, thanks. :)

That's encouraging to hear.

> > This just adds the code, subsequent patches will change the existing
> > users of line_read() to the new implementation.
> > 
> > Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
> > ---
> >  Makefile   |   8 ++--
> >  lineread.c | 108 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  lineread.h |  23 ++++++++++++
> >  3 files changed, 135 insertions(+), 4 deletions(-)
> >  create mode 100644 lineread.c
> >  create mode 100644 lineread.h
> > 
> > diff --git a/Makefile b/Makefile
> > index b0dde68..d059efb 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -32,16 +32,16 @@ CFLAGS += -DRLIMIT_STACK_VAL=$(RLIMIT_STACK_VAL)
> >  CFLAGS += -DARCH=\"$(TARGET_ARCH)\"
> >  
> >  PASST_SRCS = arch.c arp.c checksum.c conf.c dhcp.c dhcpv6.c icmp.c igmp.c \
> > -	mld.c ndp.c netlink.c packet.c passt.c pasta.c pcap.c siphash.c \
> > -	tap.c tcp.c tcp_splice.c udp.c util.c
> > +	lineread.c mld.c ndp.c netlink.c packet.c passt.c pasta.c pcap.c \
> > +	siphash.c tap.c tcp.c tcp_splice.c udp.c util.c
> >  QRAP_SRCS = qrap.c
> >  SRCS = $(PASST_SRCS) $(QRAP_SRCS)
> >  
> >  MANPAGES = passt.1 pasta.1 qrap.1
> >  
> >  PASST_HEADERS = arch.h arp.h checksum.h conf.h dhcp.h dhcpv6.h icmp.h \
> > -	ndp.h netlink.h packet.h passt.h pasta.h pcap.h siphash.h \
> > -	tap.h tcp.h tcp_splice.h udp.h util.h
> > +	lineread.h ndp.h netlink.h packet.h passt.h pasta.h pcap.h \
> > +	siphash.h tap.h tcp.h tcp_splice.h udp.h util.h
> >  HEADERS = $(PASST_HEADERS)
> >  
> >  # On gcc 11.2, with -O2 and -flto, tcp_hash() and siphash_20b(), if inlined,
> > diff --git a/lineread.c b/lineread.c
> > new file mode 100644
> > index 0000000..3e263cf
> > --- /dev/null
> > +++ b/lineread.c
> > @@ -0,0 +1,108 @@
> > +// SPDX-License-Identifier: AGPL-3.0-or-later
> > +
> > +/* PASST - Plug A Simple Socket Transport
> > + *  for qemu/UNIX domain socket mode
> > + *
> > + * PASTA - Pack A Subtle Tap Abstraction
> > + *  for network namespace/tap device mode
> > + *
> > + * lineread.c - Allocation free line-by-line buffered file input
> > + *
> > + * Copyright Red Hat
> > + * Author: David Gibson <david(a)gibson.dropbear.id.au>
> > + */
> > +
> > +#include <stddef.h>
> > +#include <fcntl.h>
> > +#include <string.h>
> > +#include <stdbool.h>
> > +#include <assert.h>
> > +#include <unistd.h>
> > +
> > +#include "lineread.h"
> > +
> > +/**
> > + * lineread_init() - Prepare for line by line file reading without allocation
> > + * @lr:		Line reader state structure to initialize
> > + * @fd:		File handle to read lines from
> 
> I think by "handle" most people refer to "FILE" stream handles, I would
> drop "handle" here or replace by "descriptor".

Fair point, adjusted.

> > + */
> > +void lineread_init(struct lineread *lr, int fd)
> > +{
> > +	lr->fd = fd;
> > +	lr->next_line = lr->count = 0;
> > +}
> > +
> > +static int peek_line(struct lineread *lr, bool eof)
> 
> This lacks a description in kerneldoc style, I would add:
> 
> /**
>  * peek_line() - Find and NULL-terminate next line in buffer
>  * @lr:		Line reader state structure
>  * @eof:	Caller indicates end-of-file was already found by read()
>  *
>  * Return: length of line in bytes, -1 if no line was found
>  */

Ok, I wasn't sure if they were considered necessary for local
functions.  Added now.

> By the way, if you're wondering why you're introducing the first usage
> of 'bool', I switched to C99 just recently. :)

Heh, ok.

> > +{
> > +	char *nl;
> > +
> > +	/* Sanity checks (which also document invariants) */
> > +	assert(lr->count >= 0);
> > +	assert(lr->next_line >= 0);
> > +	assert(lr->next_line + lr->count >= lr->next_line);
> > +	assert(lr->next_line + lr->count <= LINEREAD_BUFFER_SIZE);
> > +
> > +	nl = memchr(lr->buf + lr->next_line, '\n', lr->count);
> > +
> > +	if (nl) {
> > +		*nl = '\0';
> > +		return nl - lr->buf - lr->next_line + 1;
> 
> clang-tidy complains about the else-after-return here
> (llvm-else-after-return, readability-else-after-return), and at a
> second glance me too :) I would drop them if you're fine with it.

Fwiw, the reason I used the else style is that these are not "early"
returns in the sense that they're not handling error or other
exceptional conditions with the bulk of the function's logic to follow
in the "else" case.  Rather they're the 3 more-or-less co-equal
possible conclusions to the function.  If this were Rust, I'd drop the
explicit 'return's entirely and return the entire if expression.

That's only a very weak stylistic preference for me though, so I've
changed it to make you and the checkers happy :).

> > +	} else if (eof) {
> > +		lr->buf[lr->next_line + lr->count] = '\0';
> > +		/*
> > +		 * No trailing newline, so treat all remaining bytes
> > +		 * as the last line
> > +		 */
> > +		return lr->count;
> > +	} else {
> > +		return -1;
> > +	}
> > +}
> > +
> > +/**
> > + * lineread_get() - Read a single line from file (no allocation)
> > + * @lr:		Line reader state structure
> > + * @line:	Place a pointer to the next line in this variable
> > + *
> > + * Return:	Length of line read on success, 0 on EOF, negative on error
> > + */
> > +int lineread_get(struct lineread *lr, char **line)
> > +{
> > +	bool eof = false;
> > +	int line_len;
> > +
> > +	while ((line_len = peek_line(lr, eof)) < 0) {
> > +		int rc;
> > +
> > +		if ((lr->next_line + lr->count) == LINEREAD_BUFFER_SIZE) {
> > +			/* No space at end */
> > +			if (lr->next_line == 0) {
> > +				/*
> 
> Nit: elsewhere, I used "net" kernel style comments, which are the same
> as every other area of the Linux kernel except that there's no opening
> newline:
> 
> 				/* Buffer is full, which means we've
> 
> I would change it here and in the comment above.

Fair enough, done.

> > +				 * Buffer is full, which means we've
> > +				 * hit a line too long for us to
> > +				 * process.  FIXME: report error
> > +				 * better
> > +				 */
> > +				return -1;
> > +			}
> > +			memmove(lr->buf, lr->buf + lr->next_line, lr->count);
> > +			lr->next_line = 0;
> > +		}
> > +		
> 
> Stray tabs here, dropped.

Oops.  Usually I notice that when I commit.

> > +		/* Read more data into the end of buffer */
> > +		rc = read(lr->fd, lr->buf + lr->next_line + lr->count,
> > +			  LINEREAD_BUFFER_SIZE - lr->next_line - lr->count);
> > +		if (rc < 0) {
> > +			return rc;
> > +		} else if (rc == 0) {
> > +			eof = true;
> > +		} else {
> > +			lr->count += rc;
> > +		}
> > +	}
> > +
> > +	*line = lr->buf + lr->next_line;
> > +	lr->next_line += line_len;
> > +	lr->count -= line_len;
> > +	return line_len;
> > +}
> > diff --git a/lineread.h b/lineread.h
> > new file mode 100644
> > index 0000000..972dc51
> > --- /dev/null
> > +++ b/lineread.h
> > @@ -0,0 +1,23 @@
> > +/* SPDX-License-Identifier: AGPL-3.0-or-later
> > + * Copyright Red Hat
> > + * Author: David Gibson <david(a)gibson.dropbear.id.au>
> > + */
> > +
> > +#ifndef LINEREAD_H
> > +#define LINEREAD_H
> > +
> > +#define LINEREAD_BUFFER_SIZE	8192
> > +
> 
> I would also stick to kerneldoc style comment here:
> 
> /**
>  * struct lineread - Line reader state
>  * @fd:		File descriptor lines are read from
>  * @next_line:	...

Makes sense.

> > +struct lineread {
> > +	int fd;
> > +	int next_line; /* start of next unread line in buffer */
> > +	int count; /* number of unreturned bytes in buffer */
> > +
> > +	/* One extra byte for possible trailing \0 */
> > +	char buf[LINEREAD_BUFFER_SIZE+1];
> > +};
> > +
> > +void lineread_init(struct lineread *lr, int fd);
> > +int lineread_get(struct lineread *lr, char **line);
> > +
> > +#endif /* _LINEREAD_H */
> 
> I can change stuff on merge, let me know.

Well, I just made the changes, so I'll send a v2 shortly.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]