[PATCH v2 0/4] New line reading implementation

[PATCH v2 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 | 115 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 lineread.h |  31 +++++++++++++++
 util.c     |  64 +++--------------------------
 5 files changed, 170 insertions(+), 70 deletions(-)
 create mode 100644 lineread.c
 create mode 100644 lineread.h

-- 
2.36.1

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 6447 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 always 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 | 115 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 lineread.h |  31 +++++++++++++++
 3 files changed, 150 insertions(+), 4 deletions(-)
 create mode 100644 lineread.c
 create mode 100644 lineread.h

diff --git a/Makefile b/Makefile
index d2e25b4..c441af3 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..fca7f11
--- /dev/null
+++ b/lineread.c
@@ -0,0 +1,115 @@
+// 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 descriptor to read lines from
+ */
+void lineread_init(struct lineread *lr, int fd)
+{
+	lr->fd = fd;
+	lr->next_line = lr->count = 0;
+}
+
+/**
+ * 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
+ */
+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;
+	}
+
+	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;
+	}
+
+	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..b6faff5
--- /dev/null
+++ b/lineread.h
@@ -0,0 +1,31 @@
+/* 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 - Line reader state
+ * @fd:		File descriptor lines are read from
+ * @next_line:	Offset in @buf of the start of the first line not yet
+ *		returned by lineread_get()
+ * @count:	Number of bytes in @buf read from the file, but not yet
+ *		returned by lineread_get()
+ * @buf:	Buffer storing data read from file.
+ */
+struct lineread {
+	int fd; int next_line;
+	int count;
+
+	/* 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,31 @@
+/* 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 - Line reader state
+ * @fd:		File descriptor lines are read from
+ * @next_line:	Offset in @buf of the start of the first line not yet
+ *		returned by lineread_get()
+ * @count:	Number of bytes in @buf read from the file, but not yet
+ *		returned by lineread_get()
+ * @buf:	Buffer storing data read from file.
+ */
+struct lineread {
+	int fd; int next_line;
+	int count;
+
+	/* 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 v2 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 3a5d650..22949fd 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 v2 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 v2 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 v2 1/4] Add cleaner line-by-line reading primitives

[Stefano Brivio]

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

On Fri, 24 Jun 2022 12:17:29 +1000
David Gibson <david(a)gibson.dropbear.id.au> wrote:

> [...]
>
> +/**
> + * 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) {

clang-tidy still complains about this one, and I think it's reasonable
(though not fundamental) to change it, because (rc < 0) is a clear
error condition we're returning right away. I'd just change this on
merge.

-- 
Stefano

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

[David Gibson]

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

On Mon, Jun 27, 2022 at 12:36:32PM +0200, Stefano Brivio wrote:
> On Fri, 24 Jun 2022 12:17:29 +1000
> David Gibson <david(a)gibson.dropbear.id.au> wrote:
> 
> > [...]
> >
> > +/**
> > + * 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) {
> 
> clang-tidy still complains about this one, and I think it's reasonable
> (though not fundamental) to change it, because (rc < 0) is a clear
> error condition we're returning right away. I'd just change this on
> merge.

Fair enough.


-- 
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 --]

Re: [PATCH v2 0/4] New line reading implementation

[Stefano Brivio]

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

On Fri, 24 Jun 2022 12:17:28 +1000
David Gibson <david(a)gibson.dropbear.id.au> wrote:

> 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 | 115 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  lineread.h |  31 +++++++++++++++
>  util.c     |  64 +++--------------------------
>  5 files changed, 170 insertions(+), 70 deletions(-)
>  create mode 100644 lineread.c
>  create mode 100644 lineread.h

Applied, finally, sorry for the delay!

-- 
Stefano