[PATCH 1/3] buf: Add a generic extensible buffer struct

[PATCH 1/3] buf: Add a generic extensible buffer struct

[Andrew Zaborowski]

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

Add a generic buffer class that can extend to the left and to the right
as data is appended or prepended.  Nested buffers can also be added.
Building the complicated structures of nested genl attributes and also
various structures like 802.11 IEs passed in the contents of those
attributes is the motivation for this.

l_buf_new() and l_buf_append_new() take the amount of buffer space and
the number of nested buffers to pre-allocate but the user can go over
these values and the buffers will grow.  While the code looks a little
complicated most operations are actually simple when the buffers don't
need to grow.

I considered these few features, all of which would be nice to have
for l_genl_msg building for example, but, some of which are mutually
exclusive without blowing up the size of struct l_buf, the complexity
or the number of mallocs:

1. nested l_bufs can be added *at the head*, in addition to at the
   tail, of the parent l_buf.  (similar to how static data can be
   prepended or appended to the current l_buf)

2. multiple nested buffers of one parent can exist at the same time
   and be written into or have their own children added in random
   order.

3. l_buf contains enough information to detect when the initial
   @nested_cnt iovecs are exhausted, in l_buf_append_new().

4. on top of 3. the iov array automatically extends when the initial
   @nested_cnt iovecs are exhausted, in l_buf_append_new(), rather
   than return NULL.
   When the iov array extends parent/children nested l_bufs don't
   become invalid.

5. no extra "finalize" call is required after all data is written
   to all nested buffers and no tree structure needs to be flattened,
   the iov array is ready to use in a writev() at any time.

6. on top of 5. no l_buf_free() is needed for nested buffers.
   The number of l_buf_free calls matches the number of l_buf_new
   calls only.

This version sacrifices 1. and 2. for 3., 4., 5. and 6.
---
 Makefile.am |   6 +-
 ell/buf.c   | 192 ++++++++++++++++++++++++++++++++++++++
 ell/buf.h   | 258 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 ell/ell.h   |   1 +
 ell/ell.sym |   5 +
 5 files changed, 460 insertions(+), 2 deletions(-)
 create mode 100644 ell/buf.c
 create mode 100644 ell/buf.h

diff --git a/Makefile.am b/Makefile.am
index 2f9a4ce..93bbbf2 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -58,7 +58,8 @@ pkginclude_HEADERS = ell/ell.h \
 			ell/gpio.h \
 			ell/path.h \
 			ell/icmp6.h \
-			ell/acd.h
+			ell/acd.h \
+			ell/buf.h
 
 lib_LTLIBRARIES = ell/libell.la
 
@@ -141,7 +142,8 @@ ell_libell_la_SOURCES = $(linux_headers) \
 			ell/path.c \
 			ell/icmp6.c \
 			ell/icmp6-private.h \
-			ell/acd.c
+			ell/acd.c \
+			ell/buf.c
 
 ell_libell_la_LDFLAGS = -Wl,--no-undefined \
 			-Wl,--version-script=$(top_srcdir)/ell/ell.sym \
diff --git a/ell/buf.c b/ell/buf.c
new file mode 100644
index 0000000..8900656
--- /dev/null
+++ b/ell/buf.c
@@ -0,0 +1,192 @@
+/*
+ *
+ *  Embedded Linux library
+ *
+ *  Copyright (C) 2021  Intel Corporation. All rights reserved.
+ *
+ *  This library is free software; you can redistribute it and/or
+ *  modify it under the terms of the GNU Lesser General Public
+ *  License as published by the Free Software Foundation; either
+ *  version 2.1 of the License, or (at your option) any later version.
+ *
+ *  This library 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
+ *  Lesser General Public License for more details.
+ *
+ *  You should have received a copy of the GNU Lesser General Public
+ *  License along with this library; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <errno.h>
+
+#include "util.h"
+#include "private.h"
+#include "buf.h"
+
+LIB_EXPORT struct l_buf *l_buf_new(size_t headroom, size_t tailroom,
+					unsigned int nested_cnt)
+{
+	struct l_buf *buf;
+	unsigned int iov_cnt = 1 + nested_cnt;
+	struct iovec *alloced;
+
+	buf = l_malloc(sizeof(struct l_buf) +
+				iov_cnt * 2 * sizeof(struct iovec) +
+				headroom + tailroom);
+	buf->parent = NULL;
+	buf->child = NULL;
+	/* Point @used to the first byte after the end of the struct */
+	buf->used = buf->iovecs;
+	buf->used_cnt = 1;
+	/* Point @alloced to the first byte after the end of @used */
+	alloced = buf->used + iov_cnt;
+	buf->alloced_cnt = iov_cnt;
+	/* Point @alloced[0].iov_base to first byte after the end of @alloced */
+	alloced->iov_base = alloced + iov_cnt;
+	alloced->iov_len = headroom + tailroom;
+
+	buf->used->iov_base = alloced->iov_base + headroom;
+	buf->used->iov_len = 0;
+
+	return buf;
+}
+
+LIB_EXPORT bool l_buf_append_init(struct l_buf *buf, struct l_buf *parent,
+					size_t headroom, size_t tailroom)
+{
+	struct l_buf *base;
+
+	if (unlikely(!buf || !parent))
+		return false;
+
+	for (base = parent; base->parent; base = base->parent);
+
+	if (unlikely(base->used_cnt == base->alloced_cnt)) {
+		struct iovec *old_used = base->used;
+		struct l_buf *i;
+
+		/* Allocate a bigger buffer for the used+allocated array */
+		base->alloced_cnt *= 2;
+
+		if (old_used == base->iovecs) {
+			struct iovec *old_alloced = old_used + base->used_cnt;
+
+			/* Assign the now uneeded space to the first iovec */
+			if (old_alloced->iov_base ==
+					old_alloced + base->used_cnt) {
+				old_alloced->iov_base = old_used;
+				old_alloced->iov_len += base->used_cnt * 2 *
+                                                sizeof(struct iovec);
+			}
+
+			base->used = l_malloc(base->alloced_cnt * 2 *
+						sizeof(struct iovec));
+			memcpy(base->used, old_used,
+				base->used_cnt * sizeof(struct iovec));
+			memcpy(base->used + base->alloced_cnt, old_alloced,
+				base->used_cnt * sizeof(struct iovec));
+		} else {
+			base->used = l_realloc(base->used, base->alloced_cnt *
+						2 * sizeof(struct iovec));
+			memcpy(base->used + base->alloced_cnt,
+				base->used + base->used_cnt,
+				base->used_cnt * sizeof(struct iovec));
+		}
+
+		/* Update all intermediate l_bufs */
+		for (i = parent; i != base; i = i->parent) {
+			i->used = base->used + (i->used - old_used);
+			i->alloced_cnt = base->alloced_cnt;
+		}
+	}
+
+	buf->parent = parent;
+	buf->child = NULL;
+	buf->used = base->used + (base->used_cnt++);
+	buf->alloced_cnt = base->alloced_cnt;
+	return true;
+}
+
+LIB_EXPORT void l_buf_free(struct l_buf *buf)
+{
+	const void *static_iov_base;
+	const struct iovec *used;
+	const struct iovec *alloced;
+
+	if (unlikely(!buf || buf->parent))
+		return;
+
+	used = buf->used;
+	alloced = used + buf->alloced_cnt;
+
+	if (used == buf->iovecs)
+		static_iov_base = alloced + buf->alloced_cnt;
+	else
+		static_iov_base = buf->iovecs;
+
+	for (; buf->used_cnt; buf->used_cnt--, used++, alloced++) {
+		explicit_bzero(used->iov_base, used->iov_len);
+
+		if (alloced->iov_base != static_iov_base)
+			l_free(alloced->iov_base);
+	}
+
+	if (buf->used != buf->iovecs)
+		l_free(buf->used);
+
+	do {
+		struct l_buf *child = buf->child;
+
+		l_free(buf);
+		buf = child;
+	} while (buf);
+}
+
+LIB_EXPORT void l_buf_extend(struct l_buf *buf, size_t bytes, bool at_head)
+{
+	struct iovec *cur = buf->used;
+	struct iovec *cur_alloced = cur + buf->alloced_cnt;
+	size_t headroom = cur->iov_base - cur_alloced->iov_base;
+	const void *static_iov_base;
+
+	if (!buf->parent) {
+		if (cur == buf->iovecs)
+			static_iov_base = cur_alloced + buf->alloced_cnt;
+		else
+			static_iov_base = buf->iovecs;
+	} else
+		static_iov_base = NULL;
+
+	cur_alloced->iov_len += bytes;
+
+	/* Use realloc only if it can save us the memcpy/memmove */
+	if (cur_alloced->iov_base != static_iov_base && !at_head &&
+			cur->iov_len) {
+		cur_alloced->iov_base =
+			l_realloc(cur_alloced->iov_base, cur_alloced->iov_len);
+		cur->iov_base = cur_alloced->iov_base + headroom;
+	} else {
+		void *old_buf = cur_alloced->iov_base;
+
+		cur_alloced->iov_base = l_malloc(cur_alloced->iov_len);
+		cur->iov_base = cur_alloced->iov_base + headroom;
+
+		if (at_head)
+			cur->iov_base += bytes;
+
+		if (cur->iov_len) {
+			memcpy(cur->iov_base, old_buf + headroom, cur->iov_len);
+			explicit_bzero(old_buf + headroom, cur->iov_len);
+		}
+
+		if (old_buf != static_iov_base)
+			l_free(old_buf);
+	}
+}
diff --git a/ell/buf.h b/ell/buf.h
new file mode 100644
index 0000000..a3d64d3
--- /dev/null
+++ b/ell/buf.h
@@ -0,0 +1,258 @@
+/*
+ *
+ *  Embedded Linux library
+ *
+ *  Copyright (C) 2021  Intel Corporation. All rights reserved.
+ *
+ *  This library is free software; you can redistribute it and/or
+ *  modify it under the terms of the GNU Lesser General Public
+ *  License as published by the Free Software Foundation; either
+ *  version 2.1 of the License, or (at your option) any later version.
+ *
+ *  This library 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
+ *  Lesser General Public License for more details.
+ *
+ *  You should have received a copy of the GNU Lesser General Public
+ *  License along with this library; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#ifndef __ELL_BUF_H
+#define __ELL_BUF_H
+
+#include <sys/uio.h>
+#include <stdint.h>
+#include <stddef.h>
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/*
+ * At the low level this stores up to @alloced_cnt contiguous buffers or
+ * segments.  Within each segment there's a "used" part of the segment
+ * identified by the start pointer and the length.  There's also
+ * differentation between base l_bufs, that own the allocated memory,
+ * and nested l_bufs that point to their parent l_buf's segments.
+ *
+ * In the base l_buf, the @iovecs array contains 2 * @alloced_cnt iovecs.
+ * The last @alloced_cnt iovecs each point at the entirety of the
+ * corresponding segment's allocated memory while the first @alloced_cnt
+ * iovecs point at the "used" part of the corresponding segment.  Only
+ * segments from 0 to (@used_cnt - 1) have memory allocated.
+ *
+ * @used[0] is the "used" iovec for the "current" segment, i.e. anything
+ * between @used->iov_base and @used->iov_base + @used->iov_len.
+ * @used + @alloced_cnt points at the entirety of the "current" segment.
+ * Anything before @used->iov_base (and after @used[alloced_cnt].iov_base)
+ * is the headroom and anything after @used->iov_base + @used->iov_len
+ * (up to @used[alloced_cnt].iov_base + @used[alloced_cnt].iov_len) is
+ * the tailroom.  Initally the "used" part is empty, i.e. @used->iov_base
+ * points somewhere inside the allocated segment and @used->iov_len is 0.
+ * l_buf_prepend() is then used to expand it into the headroom and
+ * l_buf_append() to expand it into the tailroom, reducing the available
+ * room.  The headroom and tailroom of the allocated segments, and the
+ * @iov_base and @iov_len of the not-yet-allocated segments have
+ * uninitialized values.
+ *
+ * There's no way to change which segment is "current".  The segments
+ * after "current" are accessed by creating a nested l_buf using
+ * l_buf_append_init() and l_buf_append_new().  Its "current" segment is
+ * going to be different.  Segments are allocated their memory when they
+ * become the "current" segment of an l_buf.  That memory is not freed
+ * until the base l_buf is freed though.  Nested l_bufs can't be freed
+ * using l_buf_free, they're also automatically freed with the base l_buf.
+ * A nested l_buf and all its childred l_bufs also become invalid when
+ * a new call to l_buf_append_new() is made for the same parent l_buf or
+ * its ancestor.
+ */
+struct l_buf {
+	struct {
+		struct l_buf *parent;
+		struct l_buf *child;
+		struct iovec *used;
+		unsigned int alloced_cnt;
+	};
+	unsigned int used_cnt;
+	struct iovec iovecs[0];
+};
+
+struct l_buf *l_buf_new(size_t headroom, size_t tailroom,
+			unsigned int nested_cnt);
+bool l_buf_append_init(struct l_buf *buf, struct l_buf *parent,
+			size_t headroom, size_t tailroom);
+
+static inline struct l_buf *l_buf_append_new(struct l_buf *parent,
+						size_t headroom,
+						size_t tailroom)
+{
+	struct l_buf *ref_parent = parent;
+	struct l_buf *old_child;
+
+	if (unlikely(!parent))
+		return NULL;
+
+	/*
+	 * Find the first ancestor referenced by its .parent->child, i.e.
+	 * one that was allocated dynamically by l_buf_append_new().
+	 * That ancestor is going to hold the reference to the new l_buf
+	 * so that it can be automatically re-used and freed later.  Don't
+	 * link the new l_buf to a parent that is not part of the singly
+	 * linked list formed by the .child pointers, otherwise it'd leak.
+	 */
+	while (ref_parent->parent && ref_parent->parent->child != ref_parent)
+		ref_parent = ref_parent->parent;
+
+	if (ref_parent->child)
+		/* Save previous child(ren) l_buf(s) for re-use */
+		old_child = ref_parent->child->child;
+	else {
+		ref_parent->child = l_malloc(offsetof(struct l_buf, used_cnt));
+		old_child = NULL;
+	}
+
+	/* Cannot fail when parent and parent->child are non-NULL */
+	l_buf_append_init(ref_parent->child, parent, headroom, tailroom);
+
+	ref_parent->child->child = old_child;
+	return parent->child;
+}
+
+void l_buf_free(struct l_buf *buf);
+void l_buf_extend(struct l_buf *buf, size_t bytes, bool at_head);
+
+static inline uint8_t *l_buf_get_headroom(struct l_buf *buf, size_t bytes)
+{
+	struct iovec *cur = buf->used;
+	struct iovec *cur_alloced = cur + buf->alloced_cnt;
+
+	if (cur->iov_base - bytes < cur_alloced->iov_base)
+		l_buf_extend(buf, cur_alloced->iov_base + bytes - cur->iov_base,
+				true);
+
+	cur->iov_base -= bytes;
+	cur->iov_len += bytes;
+	return cur->iov_base;
+}
+
+static inline uint8_t *l_buf_get_tailroom(struct l_buf *buf, size_t bytes)
+{
+	struct iovec *cur = buf->used;
+	struct iovec *cur_alloced = cur + buf->alloced_cnt;
+	void *end = cur->iov_base + cur->iov_len;
+
+	if (end + bytes > cur_alloced->iov_base + cur_alloced->iov_len) {
+		l_buf_extend(buf, end + bytes -
+				cur_alloced->iov_base - cur_alloced->iov_len,
+				false);
+		end = cur->iov_base + cur->iov_len;
+	}
+
+	cur->iov_len += bytes;
+	return end;
+}
+
+static inline void l_buf_prepend(struct l_buf *buf, const void *data,
+					size_t len)
+{
+	if (likely(len))
+		memcpy(l_buf_get_headroom(buf, len), data, len);
+}
+
+static inline void l_buf_append(struct l_buf *buf, const void *data, size_t len)
+{
+	if (likely(len))
+		memcpy(l_buf_get_tailroom(buf, len), data, len);
+}
+
+static inline void l_buf_prependv(struct l_buf *buf, const struct iovec *iov,
+				unsigned int iov_cnt)
+{
+	while (iov_cnt) {
+		iov_cnt--;
+		l_buf_prepend(buf, iov[iov_cnt].iov_base, iov[iov_cnt].iov_len);
+	}
+}
+
+static inline void l_buf_appendv(struct l_buf *buf, const struct iovec *iov,
+				unsigned int iov_cnt)
+{
+	while (iov_cnt) {
+		iov_cnt--;
+		l_buf_append(buf, iov->iov_base, iov->iov_len);
+		iov++;
+	}
+}
+
+#define l_buf_prepends(buf, in) l_buf_prepend((buf), &(in), sizeof(in))
+#define l_buf_appends(buf, in) l_buf_append((buf), &(in), sizeof(in))
+
+static inline void l_buf_pad(struct l_buf *buf, uint8_t value, size_t len)
+{
+	if (likely(len))
+		memset(l_buf_get_tailroom(buf, len), value, len);
+}
+
+static inline size_t l_buf_get_len(const struct l_buf *buf)
+{
+	const struct l_buf *base;
+	unsigned int i;
+	size_t len = 0;
+
+	if (unlikely(!buf))
+		return 0;
+
+	for (base = buf; base->parent; base = base->parent);
+
+	for (i = buf->used - base->used; i < base->used_cnt; i++)
+		len += base->used[i].iov_len;
+
+	return len;
+}
+
+static inline bool l_buf_put_headroom(struct l_buf *buf, size_t bytes)
+{
+	if (buf->used->iov_len < bytes)
+		return false;
+
+	buf->used->iov_base += bytes;
+	buf->used->iov_len -= bytes;
+	return true;
+}
+
+static inline bool l_buf_put_tailroom(struct l_buf *buf, size_t bytes)
+{
+	if (buf->used->iov_len < bytes)
+		return false;
+
+	buf->used->iov_len -= bytes;
+	return true;
+}
+
+static inline uint8_t *l_buf_get_data(const struct l_buf *buf, size_t *out_len)
+{
+	size_t len = l_buf_get_len(buf);
+	uint8_t *data = l_malloc(len);
+	uint8_t *ptr = data;
+	int i;
+
+	if (out_len)
+		*out_len = len;
+
+	for (i = 0; len; i++) {
+		memcpy(ptr, buf->used[i].iov_base, buf->used[i].iov_len);
+		ptr += buf->used[i].iov_len;
+		len -= buf->used[i].iov_len;
+	}
+
+	return data;
+}
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* __ELL_BUF_H */
diff --git a/ell/ell.h b/ell/ell.h
index 22fddf7..ab8f0f0 100644
--- a/ell/ell.h
+++ b/ell/ell.h
@@ -64,3 +64,4 @@
 #include <ell/gpio.h>
 #include <ell/path.h>
 #include <ell/acd.h>
+#include <ell/buf.h>
diff --git a/ell/ell.sym b/ell/ell.sym
index aa0c046..8372e5f 100644
--- a/ell/ell.sym
+++ b/ell/ell.sym
@@ -664,6 +664,11 @@ global:
 	l_acd_set_debug;
 	l_acd_set_skip_probes;
 	l_acd_set_defend_policy;
+	/* buf */
+	l_buf_new;
+	l_buf_append_init;
+	l_buf_free;
+	l_buf_extend;
 local:
 	*;
 };
-- 
2.27.0

[PATCH 2/3] genl: Use l_buf for l_genl_msg building

[Andrew Zaborowski]

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

Replace l_genl_msg.data with an l_buf and update all of the users so
that genl.c doesn't need to do the size checks and reallocs.  The
function signatures are not changed except for l_genl_msg_to_data()
being replaced with l_genl_msg_get_buf().  Two new functions are added.

One little annoyance with the new code is that the order of attributes
may not be preserved as with the old code.

It would have been possible to get rid of l_genl_msg.nests[] but the
code would be slightly uglier.
---
 ell/ell.sym |   4 +-
 ell/genl.c  | 218 +++++++++++++++++++++++++++-------------------------
 ell/genl.h  |  12 ++-
 3 files changed, 124 insertions(+), 110 deletions(-)

diff --git a/ell/ell.sym b/ell/ell.sym
index 8372e5f..ee02b7a 100644
--- a/ell/ell.sym
+++ b/ell/ell.sym
@@ -297,8 +297,9 @@ global:
 	l_genl_request_family;
 	l_genl_msg_new;
 	l_genl_msg_new_sized;
+	l_genl_msg_new_bufs;
 	l_genl_msg_new_from_data;
-	l_genl_msg_to_data;
+	l_genl_msg_get_buf;
 	l_genl_msg_ref;
 	l_genl_msg_unref;
 	l_genl_msg_get_command;
@@ -307,6 +308,7 @@ global:
 	l_genl_msg_get_extended_error;
 	l_genl_msg_append_attr;
 	l_genl_msg_append_attrv;
+	l_genl_msg_append_attr_buf;
 	l_genl_msg_enter_nested;
 	l_genl_msg_leave_nested;
 	l_genl_attr_init;
diff --git a/ell/genl.c b/ell/genl.c
index a4f7252..7e6f2f1 100644
--- a/ell/genl.c
+++ b/ell/genl.c
@@ -34,6 +34,7 @@
 #include "io.h"
 #include "netlink-private.h"
 #include "genl.h"
+#include "buf.h"
 #include "private.h"
 
 #define MAX_NESTING_LEVEL 4
@@ -43,7 +44,7 @@
 
 struct nest_info {
 	uint16_t type;
-	uint16_t offset;
+	struct l_buf *parent_buf;
 };
 
 struct unicast_watch {
@@ -104,11 +105,13 @@ struct l_genl_msg {
 	char *error_msg;
 	uint8_t cmd;
 	uint8_t version;
-	void *data;
 	uint32_t size;
-	uint32_t len;
+	struct l_buf *buf;
+	struct l_buf *cur_buf;
 	struct nest_info nests[MAX_NESTING_LEVEL];
 	uint8_t nesting_level;
+	uint16_t attr_type;
+	struct l_buf *attr_buf;
 };
 
 struct genl_request {
@@ -740,7 +743,8 @@ static bool match_request_hid(const void *a, const void *b)
 #define NLA_DATA(nla)		((void*)(((char*)(nla)) + NLA_LENGTH(0)))
 #define NLA_PAYLOAD(nla)	((int)((nla)->nla_len) - NLA_LENGTH(0))
 
-static struct l_genl_msg *msg_alloc(uint8_t cmd, uint8_t version, uint32_t size)
+static struct l_genl_msg *msg_alloc(uint8_t cmd, uint8_t version, uint32_t size,
+					uint32_t n_bufs)
 {
 	struct l_genl_msg *msg;
 
@@ -749,35 +753,15 @@ static struct l_genl_msg *msg_alloc(uint8_t cmd, uint8_t version, uint32_t size)
 	msg->cmd = cmd;
 	msg->version = version;
 
-	msg->len = NLMSG_HDRLEN + GENL_HDRLEN;
-	msg->size = msg->len + NLMSG_ALIGN(size);
-
-	msg->data = l_realloc(NULL, msg->size);
-	memset(msg->data, 0, msg->size);
+	msg->size = NLMSG_ALIGN(size);
+	msg->buf = l_buf_new(NLMSG_HDRLEN + GENL_HDRLEN, msg->size, n_bufs);
+	msg->cur_buf = msg->buf;
 	msg->nesting_level = 0;
+	msg->attr_buf = NULL;
 
 	return l_genl_msg_ref(msg);
 }
 
-static bool msg_grow(struct l_genl_msg *msg, uint32_t needed)
-{
-	uint32_t grow_by;
-
-	if (msg->size >= msg->len + needed)
-		return true;
-
-	grow_by = msg->len + needed - msg->size;
-
-	if (grow_by < 32)
-		grow_by = 128;
-
-	msg->data = l_realloc(msg->data, msg->size + grow_by);
-	memset(msg->data + msg->size, 0, grow_by);
-	msg->size += grow_by;
-
-	return true;
-}
-
 static struct l_genl_msg *msg_create(const struct nlmsghdr *nlmsg)
 {
 	struct l_genl_msg *msg;
@@ -828,13 +812,13 @@ static struct l_genl_msg *msg_create(const struct nlmsghdr *nlmsg)
 		}
 	}
 
-	msg->data = l_memdup(nlmsg, nlmsg->nlmsg_len);
-
-	msg->len = nlmsg->nlmsg_len;
+	msg->buf = l_buf_new(0, nlmsg->nlmsg_len, 0);
 	msg->size = nlmsg->nlmsg_len;
 
-	if (msg->len >= GENL_HDRLEN) {
-		struct genlmsghdr *genlmsg = msg->data + NLMSG_HDRLEN;
+	l_buf_append(msg->buf, nlmsg, nlmsg->nlmsg_len);
+
+	if (msg->size >= GENL_HDRLEN) {
+		struct genlmsghdr *genlmsg = msg->buf->used->iov_base + NLMSG_HDRLEN;
 
 		msg->cmd = genlmsg->cmd;
 		msg->version = genlmsg->version;
@@ -844,30 +828,25 @@ done:
 	return l_genl_msg_ref(msg);
 }
 
-static const void *msg_as_bytes(struct l_genl_msg *msg, uint16_t type,
-				uint16_t flags, uint32_t seq, uint32_t pid,
-				size_t *out_size)
+static void genl_msg_write_header(struct l_genl_msg *msg, uint16_t type,
+					uint16_t flags, uint32_t seq,
+					uint32_t pid)
 {
-	struct nlmsghdr *nlmsg;
-	struct genlmsghdr *genlmsg;
-
-	nlmsg = msg->data;
-
-	nlmsg->nlmsg_len = msg->len;
-	nlmsg->nlmsg_type = type;
-	nlmsg->nlmsg_flags = flags;
-	nlmsg->nlmsg_seq = seq;
-	nlmsg->nlmsg_pid = pid;
-
-	genlmsg = msg->data + NLMSG_HDRLEN;
-
-	genlmsg->cmd = msg->cmd;
-	genlmsg->version = msg->version;
+	struct nlmsghdr nlmsg = {
+		.nlmsg_len = l_buf_get_len(msg->buf) +
+			NLMSG_HDRLEN + GENL_HDRLEN,
+		.nlmsg_type = type,
+		.nlmsg_flags = flags,
+		.nlmsg_seq = seq,
+		.nlmsg_pid = pid,
+	};
+	struct genlmsghdr genlmsg = {
+		.cmd = msg->cmd,
+		.version = msg->version,
+	};
 
-	if (out_size)
-		*out_size = msg->len;
-
-	return msg->data;
+	l_buf_prepend(msg->buf, &genlmsg, GENL_HDRLEN);
+	l_buf_prepend(msg->buf, &nlmsg, NLMSG_HDRLEN);
 }
 
 static void write_watch_destroy(void *user_data)
@@ -881,25 +860,29 @@ static bool can_write_data(struct l_io *io, void *user_data)
 {
 	struct l_genl *genl = user_data;
 	struct genl_request *request;
-	const void *data;
-	size_t size;
 	ssize_t bytes_written;
+	unsigned int i;
 
 	request = l_queue_pop_head(genl->request_queue);
 	if (!request)
 		return false;
 
 	request->seq = get_next_id(&genl->next_seq);
-	data = msg_as_bytes(request->msg, request->type, request->flags,
-				request->seq, genl->pid, &size);
+	genl_msg_write_header(request->msg, request->type, request->flags,
+				request->seq, genl->pid);
 
-	bytes_written = send(genl->fd, data, size, 0);
+	bytes_written = writev(genl->fd, request->msg->buf->used,
+				request->msg->buf->used_cnt);
 	if (bytes_written < 0) {
+		l_buf_put_headroom(request->msg->buf,
+					GENL_HDRLEN + NLMSG_HDRLEN);
 		l_queue_push_head(genl->request_queue, request);
 		return false;
 	}
 
-	l_util_hexdump(false, request->msg->data, bytes_written,
+	for (i = 0; i < request->msg->buf->used_cnt; i++)
+		l_util_hexdump(false, request->msg->buf->used[i].iov_base,
+				request->msg->buf->used[i].iov_len,
 				genl->debug_callback, genl->debug_data);
 
 	l_queue_push_tail(genl->pending_list, request);
@@ -1567,7 +1550,12 @@ LIB_EXPORT struct l_genl_msg *l_genl_msg_new(uint8_t cmd)
 
 LIB_EXPORT struct l_genl_msg *l_genl_msg_new_sized(uint8_t cmd, uint32_t size)
 {
-	return msg_alloc(cmd, 0x00, size);
+	return msg_alloc(cmd, 0x00, size, 16);
+}
+
+LIB_EXPORT struct l_genl_msg *l_genl_msg_new_bufs(uint8_t cmd, uint32_t n_bufs)
+{
+	return msg_alloc(cmd, 0x00, 256, n_bufs);
 }
 
 LIB_EXPORT struct l_genl_msg *l_genl_msg_new_from_data(const void *data,
@@ -1584,12 +1572,15 @@ LIB_EXPORT struct l_genl_msg *l_genl_msg_new_from_data(const void *data,
 	return msg_create(nlmsg);
 }
 
-LIB_EXPORT const void *l_genl_msg_to_data(struct l_genl_msg *msg, uint16_t type,
-						uint16_t flags, uint32_t seq,
-						uint32_t pid,
-						size_t *out_size)
+LIB_EXPORT const struct l_buf *l_genl_msg_get_buf(struct l_genl_msg *msg,
+							uint16_t type,
+							uint16_t flags,
+							uint32_t seq,
+							uint32_t pid)
 {
-	return msg_as_bytes(msg, type, flags, seq, pid, out_size);
+	genl_msg_write_header(msg, type, flags, seq, pid);
+
+	return msg->buf;
 }
 
 LIB_EXPORT struct l_genl_msg *l_genl_msg_ref(struct l_genl_msg *msg)
@@ -1611,7 +1602,7 @@ LIB_EXPORT void l_genl_msg_unref(struct l_genl_msg *msg)
 		return;
 
 	l_free(msg->error_msg);
-	l_free(msg->data);
+	l_buf_free(msg->buf);
 	l_free(msg);
 }
 
@@ -1650,23 +1641,17 @@ LIB_EXPORT const char *l_genl_msg_get_extended_error(struct l_genl_msg *msg)
 LIB_EXPORT bool l_genl_msg_append_attr(struct l_genl_msg *msg, uint16_t type,
 						uint16_t len, const void *data)
 {
-	struct nlattr *nla;
+	struct nlattr nla = {
+		.nla_len = NLA_HDRLEN + len,
+		.nla_type = type,
+	};
 
 	if (unlikely(!msg))
 		return false;
 
-	if (!msg_grow(msg, NLA_HDRLEN + NLA_ALIGN(len)))
-		return false;
-
-	nla = msg->data + msg->len;
-	nla->nla_len = NLA_HDRLEN + len;
-	nla->nla_type = type;
-
-	if (len)
-		memcpy(msg->data + msg->len + NLA_HDRLEN, data, len);
-
-	msg->len += NLA_HDRLEN + NLA_ALIGN(len);
-
+	l_buf_append(msg->cur_buf, &nla, NLA_HDRLEN);
+	l_buf_append(msg->cur_buf, data, len);
+	l_buf_pad(msg->cur_buf, 0, NLA_ALIGN(len) - len);
 	return true;
 }
 
@@ -1674,7 +1659,7 @@ LIB_EXPORT bool l_genl_msg_append_attrv(struct l_genl_msg *msg, uint16_t type,
 					const struct iovec *iov,
 					size_t iov_len)
 {
-	struct nlattr *nla;
+	struct nlattr nla = { .nla_type = type };
 	size_t len = 0;
 	unsigned int i;
 
@@ -1684,23 +1669,26 @@ LIB_EXPORT bool l_genl_msg_append_attrv(struct l_genl_msg *msg, uint16_t type,
 	for (i = 0; i < iov_len; i++)
 		len += iov[i].iov_len;
 
-	if (!msg_grow(msg, NLA_HDRLEN + NLA_ALIGN(len)))
-		return false;
+	nla.nla_len = NLA_HDRLEN + len;
 
-	nla = msg->data + msg->len;
-	nla->nla_len = NLA_HDRLEN + len;
-	nla->nla_type = type;
+	l_buf_append(msg->cur_buf, &nla, NLA_HDRLEN);
+	l_buf_appendv(msg->cur_buf, iov, iov_len);
+	l_buf_pad(msg->cur_buf, 0, NLA_ALIGN(len) - len);
+	return true;
+}
 
-	msg->len += NLA_HDRLEN;
+static void genl_msg_write_attr_header(struct l_genl_msg *msg)
+{
+	struct nlattr nla = {};
 
-	for (i = 0; i < iov_len; i++, iov++) {
-		memcpy(msg->data + msg->len, iov->iov_base, iov->iov_len);
-		msg->len += iov->iov_len;
-	}
+	if (!msg->attr_buf)
+		return;
 
-	msg->len += NLA_ALIGN(len) - len;
+	nla.nla_type = msg->attr_type;
+	nla.nla_len = l_buf_get_len(msg->attr_buf) + NLA_HDRLEN;
+	l_buf_prepend(msg->attr_buf, &nla, NLA_HDRLEN);
 
-	return true;
+	msg->attr_buf = NULL;
 }
 
 LIB_EXPORT bool l_genl_msg_enter_nested(struct l_genl_msg *msg, uint16_t type)
@@ -1711,21 +1699,19 @@ LIB_EXPORT bool l_genl_msg_enter_nested(struct l_genl_msg *msg, uint16_t type)
 	if (unlikely(msg->nesting_level == MAX_NESTING_LEVEL))
 		return false;
 
-	if (!msg_grow(msg, NLA_HDRLEN))
-		return false;
-
 	msg->nests[msg->nesting_level].type = type | NLA_F_NESTED;
-	msg->nests[msg->nesting_level].offset = msg->len;
+	msg->nests[msg->nesting_level].parent_buf = msg->cur_buf;
 	msg->nesting_level += 1;
 
-	msg->len += NLA_HDRLEN;
+	genl_msg_write_attr_header(msg);
+	msg->cur_buf = l_buf_append_new(msg->cur_buf, NLA_HDRLEN, msg->size);
 
 	return true;
 }
 
 LIB_EXPORT bool l_genl_msg_leave_nested(struct l_genl_msg *msg)
 {
-	struct nlattr *nla;
+	struct nlattr nla = {};
 
 	if (unlikely(!msg))
 		return false;
@@ -1733,15 +1719,33 @@ LIB_EXPORT bool l_genl_msg_leave_nested(struct l_genl_msg *msg)
 	if (unlikely(msg->nesting_level == 0))
 		return false;
 
-	nla = msg->data + msg->nests[msg->nesting_level - 1].offset;
-	nla->nla_type = msg->nests[msg->nesting_level - 1].type;
-	nla->nla_len = msg->len - msg->nests[msg->nesting_level - 1].offset;
+	genl_msg_write_attr_header(msg);
 
+	nla.nla_type = msg->nests[msg->nesting_level - 1].type;
+	nla.nla_len = l_buf_get_len(msg->cur_buf) + NLA_HDRLEN;
+	l_buf_prepend(msg->cur_buf, &nla, NLA_HDRLEN);
+
+	msg->cur_buf = msg->nests[msg->nesting_level - 1].parent_buf;
 	msg->nesting_level -= 1;
 
 	return true;
 }
 
+LIB_EXPORT struct l_buf *l_genl_msg_append_attr_buf(struct l_genl_msg *msg,
+							uint16_t type,
+							size_t headroom,
+							size_t tailroom)
+{
+	if (unlikely(!msg))
+		return NULL;
+
+	genl_msg_write_attr_header(msg);
+	msg->attr_buf = l_buf_append_new(msg->cur_buf, headroom + NLA_HDRLEN,
+						tailroom);
+
+	return msg->attr_buf;
+}
+
 LIB_EXPORT bool l_genl_attr_init(struct l_genl_attr *attr,
 						struct l_genl_msg *msg)
 {
@@ -1751,11 +1755,11 @@ LIB_EXPORT bool l_genl_attr_init(struct l_genl_attr *attr,
 	if (unlikely(!attr) || unlikely(!msg))
 		return false;
 
-	if (!msg->data || msg->len < NLMSG_HDRLEN + GENL_HDRLEN)
+	if (msg->buf->used->iov_len < NLMSG_HDRLEN + GENL_HDRLEN)
 		return false;
 
-	nla = msg->data + NLMSG_HDRLEN + GENL_HDRLEN;
-	len = msg->len - NLMSG_HDRLEN - GENL_HDRLEN;
+	nla = msg->buf->used->iov_base + NLMSG_HDRLEN + GENL_HDRLEN;
+	len = msg->buf->used->iov_len - NLMSG_HDRLEN - GENL_HDRLEN;
 
 	if (!NLA_OK(nla, len))
 		return false;
@@ -1898,6 +1902,8 @@ static unsigned int send_common(struct l_genl_family *family, uint16_t flags,
 	if (!genl)
 		return 0;
 
+	genl_msg_write_attr_header(msg);
+
 	request = l_new(struct genl_request, 1);
 	request->type = family->id;
 	request->flags = NLM_F_REQUEST | flags;
diff --git a/ell/genl.h b/ell/genl.h
index 7550bf8..14e8b36 100644
--- a/ell/genl.h
+++ b/ell/genl.h
@@ -37,6 +37,7 @@ struct l_genl;
 struct l_genl_family_info;
 struct l_genl_family;
 struct l_genl_msg;
+struct l_buf;
 
 typedef void (*l_genl_destroy_func_t)(void *user_data);
 typedef void (*l_genl_debug_func_t)(const char *str, void *user_data);
@@ -85,11 +86,12 @@ struct l_genl_attr {
 
 struct l_genl_msg* l_genl_msg_new(uint8_t cmd);
 struct l_genl_msg *l_genl_msg_new_sized(uint8_t cmd, uint32_t size);
+struct l_genl_msg *l_genl_msg_new_bufs(uint8_t cmd, uint32_t n_bufs);
 struct l_genl_msg *l_genl_msg_new_from_data(const void *data, size_t size);
 
-const void *l_genl_msg_to_data(struct l_genl_msg *msg, uint16_t type,
-				uint16_t flags, uint32_t seq, uint32_t pid,
-				size_t *out_size);
+const struct l_buf *l_genl_msg_get_buf(struct l_genl_msg *msg,
+					uint16_t type, uint16_t flags,
+					uint32_t seq, uint32_t pid);
 
 struct l_genl_msg *l_genl_msg_ref(struct l_genl_msg *msg);
 void l_genl_msg_unref(struct l_genl_msg *msg);
@@ -103,6 +105,10 @@ bool l_genl_msg_append_attr(struct l_genl_msg *msg, uint16_t type,
 					uint16_t len, const void *data);
 bool l_genl_msg_append_attrv(struct l_genl_msg *msg, uint16_t type,
 				const struct iovec *iov, size_t iov_len);
+struct l_buf *l_genl_msg_append_attr_buf(struct l_genl_msg *msg,
+						uint16_t type,
+						size_t headroom,
+						size_t tailroom);
 bool l_genl_msg_enter_nested(struct l_genl_msg *msg, uint16_t type);
 bool l_genl_msg_leave_nested(struct l_genl_msg *msg);
 
-- 
2.27.0

[PATCH 3/3] unit: Update genl tests after l_genl_msg_to_data removal

[Andrew Zaborowski]

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

---
 unit/test-genl-msg.c | 33 +++++++++++++++++++++++++--------
 1 file changed, 25 insertions(+), 8 deletions(-)

diff --git a/unit/test-genl-msg.c b/unit/test-genl-msg.c
index 1653da1..ed286cc 100644
--- a/unit/test-genl-msg.c
+++ b/unit/test-genl-msg.c
@@ -101,8 +101,9 @@ static void build_set_station(const void *data)
 		{ 0x24, 0xa2, 0xe1, 0xec, 0x17, 0x04 };
 	static uint32_t flags[] = { 2, 2 };
 	struct l_genl_msg *msg;
-	const void *raw;
+	void *raw;
 	size_t size;
+	const struct l_buf *buf;
 
 	msg = l_genl_msg_new_sized(18, 512);
 	assert(msg);
@@ -111,7 +112,8 @@ static void build_set_station(const void *data)
 	assert(l_genl_msg_append_attr(msg, 6, 6, mac));
 	assert(l_genl_msg_append_attr(msg, 67, 8, flags));
 
-	raw = l_genl_msg_to_data(msg, 0x17, 0x05, 0x550d538b, 3604, &size);
+	buf = l_genl_msg_get_buf(msg, 0x17, 0x05, 0x550d538b, 3604);
+	raw = l_buf_get_data(buf, &size);
 
 	if (do_print) {
 		l_util_hexdump(false, raw, size, do_debug, "[MSG] ");
@@ -122,6 +124,7 @@ static void build_set_station(const void *data)
 	assert(size == sizeof(set_station_request));
 	assert(!memcmp(raw, set_station_request, size));
 
+	l_free(raw);
 	l_genl_msg_unref(msg);
 }
 
@@ -215,8 +218,9 @@ static void build_set_rekey_offload(const void *data)
 	static const unsigned char replay_counter[] = {
 		0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01 };
 	struct l_genl_msg *msg;
-	const void *raw;
+	void *raw;
 	size_t size;
+	const struct l_buf *buf;
 
 	msg = l_genl_msg_new_sized(79, 0);
 	assert(msg);
@@ -229,8 +233,9 @@ static void build_set_rekey_offload(const void *data)
 	assert(l_genl_msg_append_attr(msg, 3, 8, replay_counter));
 	assert(l_genl_msg_leave_nested(msg));
 
-	raw = l_genl_msg_to_data(msg, 0x1b, 0x05, 0x53e1a359, 0xe74002ba,
-					&size);
+	buf = l_genl_msg_get_buf(msg, 0x1b, 0x05, 0x53e1a359, 0xe74002ba);
+	raw = l_buf_get_data(buf, &size);
+
 	if (do_print) {
 		l_util_hexdump(false, raw, size, do_debug, "[MSG] ");
 		l_util_hexdump(true, set_rekey_offload_request, size,
@@ -240,6 +245,7 @@ static void build_set_rekey_offload(const void *data)
 	assert(size == sizeof(set_rekey_offload_request));
 	assert(!memcmp(raw, set_rekey_offload_request, size));
 
+	l_free(raw);
 	l_genl_msg_unref(msg);
 }
 
@@ -335,8 +341,9 @@ static void build_libnl_nested(const void *data)
 {
 	static uint32_t index = 1;
 	struct l_genl_msg *msg;
-	const void *raw;
+	void *raw;
 	size_t size;
+	const struct l_buf *buf;
 
 	msg = l_genl_msg_new_sized(1, 16);
 	assert(msg);
@@ -352,8 +359,9 @@ static void build_libnl_nested(const void *data)
 	assert(l_genl_msg_leave_nested(msg));
 	assert(l_genl_msg_leave_nested(msg));
 
-	raw = l_genl_msg_to_data(msg, 0x15, 0x05, 0x55130572, 0x0c406877,
-					&size);
+	buf = l_genl_msg_get_buf(msg, 0x15, 0x05, 0x55130572, 0x0c406877);
+	raw = l_buf_get_data(buf, &size);
+
 	if (do_print) {
 		l_util_hexdump(false, raw, size, do_debug, "[MSG] ");
 		l_util_hexdump(true, libnl_nested, sizeof(libnl_nested),
@@ -363,6 +371,7 @@ static void build_libnl_nested(const void *data)
 	assert(size == sizeof(libnl_nested));
 	assert(!memcmp(raw, libnl_nested, size));
 
+	l_free(raw);
 	l_genl_msg_unref(msg);
 }
 
@@ -393,6 +402,8 @@ static void test_append_attrv(const void *data)
 	uint16_t attr_type;
 	uint16_t attr_len;
 	const void *attr_payload;
+	void *raw;
+	size_t size;
 
 	assert(L_ARRAY_SIZE(iov) == num_blocks);
 
@@ -400,6 +411,12 @@ static void test_append_attrv(const void *data)
 	assert(msg);
 
 	assert(l_genl_msg_append_attrv(msg, type, iov, L_ARRAY_SIZE(iov)));
+	raw = l_buf_get_data(l_genl_msg_get_buf(msg, 0, 0, 0, 0), &size);
+	l_genl_msg_unref(msg);
+
+	msg = l_genl_msg_new_from_data(raw, size);
+	l_free(raw);
+	assert(msg);
 	assert(l_genl_attr_init(&attr, msg));
 	assert(l_genl_attr_next(&attr, &attr_type, &attr_len, &attr_payload));
 
-- 
2.27.0

Re: [PATCH 1/3] buf: Add a generic extensible buffer struct

[Mat Martineau]

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

On Fri, 8 Jan 2021, Andrew Zaborowski wrote:

> Add a generic buffer class that can extend to the left and to the right
> as data is appended or prepended.  Nested buffers can also be added.
> Building the complicated structures of nested genl attributes and also
> various structures like 802.11 IEs passed in the contents of those
> attributes is the motivation for this.
>
> l_buf_new() and l_buf_append_new() take the amount of buffer space and
> the number of nested buffers to pre-allocate but the user can go over
> these values and the buffers will grow.  While the code looks a little
> complicated most operations are actually simple when the buffers don't
> need to grow.
>
> I considered these few features, all of which would be nice to have
> for l_genl_msg building for example, but, some of which are mutually
> exclusive without blowing up the size of struct l_buf, the complexity
> or the number of mallocs:
>
> 1. nested l_bufs can be added *at the head*, in addition to at the
>   tail, of the parent l_buf.  (similar to how static data can be
>   prepended or appended to the current l_buf)
>
> 2. multiple nested buffers of one parent can exist at the same time
>   and be written into or have their own children added in random
>   order.
>
> 3. l_buf contains enough information to detect when the initial
>   @nested_cnt iovecs are exhausted, in l_buf_append_new().
>
> 4. on top of 3. the iov array automatically extends when the initial
>   @nested_cnt iovecs are exhausted, in l_buf_append_new(), rather
>   than return NULL.
>   When the iov array extends parent/children nested l_bufs don't
>   become invalid.
>
> 5. no extra "finalize" call is required after all data is written
>   to all nested buffers and no tree structure needs to be flattened,
>   the iov array is ready to use in a writev() at any time.
>
> 6. on top of 5. no l_buf_free() is needed for nested buffers.
>   The number of l_buf_free calls matches the number of l_buf_new
>   calls only.
>
> This version sacrifices 1. and 2. for 3., 4., 5. and 6.
> ---
> Makefile.am |   6 +-
> ell/buf.c   | 192 ++++++++++++++++++++++++++++++++++++++
> ell/buf.h   | 258 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> ell/ell.h   |   1 +
> ell/ell.sym |   5 +
> 5 files changed, 460 insertions(+), 2 deletions(-)
> create mode 100644 ell/buf.c
> create mode 100644 ell/buf.h
>

...

> diff --git a/ell/buf.h b/ell/buf.h
> new file mode 100644
> index 0000000..a3d64d3
> --- /dev/null
> +++ b/ell/buf.h
> @@ -0,0 +1,258 @@
> +/*
> + *
> + *  Embedded Linux library
> + *
> + *  Copyright (C) 2021  Intel Corporation. All rights reserved.
> + *
> + *  This library is free software; you can redistribute it and/or
> + *  modify it under the terms of the GNU Lesser General Public
> + *  License as published by the Free Software Foundation; either
> + *  version 2.1 of the License, or (at your option) any later version.
> + *
> + *  This library 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
> + *  Lesser General Public License for more details.
> + *
> + *  You should have received a copy of the GNU Lesser General Public
> + *  License along with this library; if not, write to the Free Software
> + *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> + *
> + */
> +
> +#ifndef __ELL_BUF_H
> +#define __ELL_BUF_H
> +
> +#include <sys/uio.h>
> +#include <stdint.h>
> +#include <stddef.h>
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +/*
> + * At the low level this stores up to @alloced_cnt contiguous buffers or
> + * segments.  Within each segment there's a "used" part of the segment
> + * identified by the start pointer and the length.  There's also
> + * differentation between base l_bufs, that own the allocated memory,
> + * and nested l_bufs that point to their parent l_buf's segments.
> + *
> + * In the base l_buf, the @iovecs array contains 2 * @alloced_cnt iovecs.
> + * The last @alloced_cnt iovecs each point at the entirety of the
> + * corresponding segment's allocated memory while the first @alloced_cnt
> + * iovecs point at the "used" part of the corresponding segment.  Only
> + * segments from 0 to (@used_cnt - 1) have memory allocated.
> + *
> + * @used[0] is the "used" iovec for the "current" segment, i.e. anything
> + * between @used->iov_base and @used->iov_base + @used->iov_len.
> + * @used + @alloced_cnt points at the entirety of the "current" segment.
> + * Anything before @used->iov_base (and after @used[alloced_cnt].iov_base)
> + * is the headroom and anything after @used->iov_base + @used->iov_len
> + * (up to @used[alloced_cnt].iov_base + @used[alloced_cnt].iov_len) is
> + * the tailroom.  Initally the "used" part is empty, i.e. @used->iov_base
> + * points somewhere inside the allocated segment and @used->iov_len is 0.
> + * l_buf_prepend() is then used to expand it into the headroom and
> + * l_buf_append() to expand it into the tailroom, reducing the available
> + * room.  The headroom and tailroom of the allocated segments, and the
> + * @iov_base and @iov_len of the not-yet-allocated segments have
> + * uninitialized values.
> + *
> + * There's no way to change which segment is "current".  The segments
> + * after "current" are accessed by creating a nested l_buf using
> + * l_buf_append_init() and l_buf_append_new().  Its "current" segment is
> + * going to be different.  Segments are allocated their memory when they
> + * become the "current" segment of an l_buf.  That memory is not freed
> + * until the base l_buf is freed though.  Nested l_bufs can't be freed
> + * using l_buf_free, they're also automatically freed with the base l_buf.
> + * A nested l_buf and all its childred l_bufs also become invalid when
> + * a new call to l_buf_append_new() is made for the same parent l_buf or
> + * its ancestor.
> + */
> +struct l_buf {
> +	struct {
> +		struct l_buf *parent;
> +		struct l_buf *child;
> +		struct iovec *used;
> +		unsigned int alloced_cnt;
> +	};
> +	unsigned int used_cnt;
> +	struct iovec iovecs[0];
> +};

Hi Andrew,

I was expecting to see an opaque structure for l_buf here in the header - 
that seems more typical for ELL!

Looking at patch #2 in the series, the genl code is mainly accessing 
buf->used and buf->used_cnt. My recommendation to make the API fit in ELL 
better, and to clarify the API, would be to make struct l_buf opaque and 
add some accessors that just return buf->used and buf->used_cnt - maybe 
l_buf_get_iov_base() and l_buf_get_iov_cnt()?

Alternately, if some of the inline functions in buf.h that access the 
struct members really need to be inline, the structure could remain 
exposed. I lean toward moving a bunch of that inline code to buf.c though.

Unit tests for l_buf would be good too.


Thanks!


--
Mat Martineau
Intel

Re: [PATCH 1/3] buf: Add a generic extensible buffer struct

[Andrew Zaborowski]

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

Hi Mat, thanks for reviewing this.

On Wed, 13 Jan 2021 at 02:00, Mat Martineau
<mathew.j.martineau@linux.intel.com> wrote:
> On Fri, 8 Jan 2021, Andrew Zaborowski wrote:
> > +struct l_buf {
> > +     struct {
> > +             struct l_buf *parent;
> > +             struct l_buf *child;
> > +             struct iovec *used;
> > +             unsigned int alloced_cnt;
> > +     };
> > +     unsigned int used_cnt;
> > +     struct iovec iovecs[0];
> > +};
>
> I was expecting to see an opaque structure for l_buf here in the header -
> that seems more typical for ELL!

Right, most real "classes" in ell are opaque in this sense.  I felt
that the low level nature of this utility and the fact that it
replaces basic (c builtin) mechanisms guarantees it being just a
struct, like the skbufs in the kernel.

I don't know how much effect inlining functions really has, even in
hot paths.  The other
feature here that uses the struct definition is that we allow the
nested l_bufs to be allocated on stack.  For example:

struct l_buf *attr_buf = l_genl_msg_append_attr_buf(msg,
NL80211_ATTR_IE, 100, 100);

if (is_rsn) {
  struct ie_rsn_info rsn = { ... };
  struct l_buf rsn_buf; /* On stack */

  l_buf_append_init(&rsn_buf, attr_buf, 50, 50);
  ie_build_rnse(&rsn_buf, &rsn);
  /* This is made up.  We can have ie_build_rsne allocate rsn_buf
internally.  It could also append directly to attr_buf but in that
case it can't *prepend* data and has to build it sequentially. */
}

/* This is equivalent: */
if (is_rsn) {
   struct ie_rsn_info rsn = { ... };
  struct l_buf *rsn_buf = l_buf_append_new(attr_buf, 50, 50);
  /* Here allocated dynamically, although on the next
l_buf_append_new() call for attr_buf, we'd be re-using the same memory
and same pointer (as documented the old one becomes invalid) so it's
not a big difference */

  ie_build_rnse(rsn_buf, &rsn);
}

We can do without this feature obviously.  I didn't bother with the
unit tests yet because this is just an RFC for now.

Best regards

Re: [PATCH 1/3] buf: Add a generic extensible buffer struct

[Mat Martineau]

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

On Thu, 14 Jan 2021, Andrew Zaborowski wrote:

> Hi Mat, thanks for reviewing this.
>
> On Wed, 13 Jan 2021 at 02:00, Mat Martineau
> <mathew.j.martineau@linux.intel.com> wrote:
>> On Fri, 8 Jan 2021, Andrew Zaborowski wrote:
>>> +struct l_buf {
>>> +     struct {
>>> +             struct l_buf *parent;
>>> +             struct l_buf *child;
>>> +             struct iovec *used;
>>> +             unsigned int alloced_cnt;
>>> +     };
>>> +     unsigned int used_cnt;
>>> +     struct iovec iovecs[0];
>>> +};
>>
>> I was expecting to see an opaque structure for l_buf here in the header -
>> that seems more typical for ELL!
>
> Right, most real "classes" in ell are opaque in this sense.  I felt
> that the low level nature of this utility and the fact that it
> replaces basic (c builtin) mechanisms guarantees it being just a
> struct, like the skbufs in the kernel.
>
> I don't know how much effect inlining functions really has, even in
> hot paths.  The other
> feature here that uses the struct definition is that we allow the
> nested l_bufs to be allocated on stack.  For example:
>
> struct l_buf *attr_buf = l_genl_msg_append_attr_buf(msg,
> NL80211_ATTR_IE, 100, 100);
>
> if (is_rsn) {
>  struct ie_rsn_info rsn = { ... };
>  struct l_buf rsn_buf; /* On stack */
>
>  l_buf_append_init(&rsn_buf, attr_buf, 50, 50);
>  ie_build_rnse(&rsn_buf, &rsn);
>  /* This is made up.  We can have ie_build_rsne allocate rsn_buf
> internally.  It could also append directly to attr_buf but in that
> case it can't *prepend* data and has to build it sequentially. */
> }
>
> /* This is equivalent: */
> if (is_rsn) {
>   struct ie_rsn_info rsn = { ... };
>  struct l_buf *rsn_buf = l_buf_append_new(attr_buf, 50, 50);
>  /* Here allocated dynamically, although on the next
> l_buf_append_new() call for attr_buf, we'd be re-using the same memory
> and same pointer (as documented the old one becomes invalid) so it's
> not a big difference */
>
>  ie_build_rnse(rsn_buf, &rsn);
> }
>
> We can do without this feature obviously.  I didn't bother with the
> unit tests yet because this is just an RFC for now.

Thanks for the additional background, and for letting me know it's still 
RFC-stage.

I think what it comes down to for me is that exposing the struct internals 
locks those in as part of the "public API" that would be harder to change 
in the future, and that it's less clear how to best use l_buf.

While I lean toward the opaque struct instead of allowing on-stack use, I 
don't know if I convinced you or Denis to lean the same way :)

Thanks for the patches, I think it's a useful feature for ELL.

--
Mat Martineau
Intel

Re: [PATCH 1/3] buf: Add a generic extensible buffer struct

[Denis Kenzior]

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

Hi Mat,

> Thanks for the additional background, and for letting me know it's still RFC-stage.
> 
> I think what it comes down to for me is that exposing the struct internals locks 
> those in as part of the "public API" that would be harder to change in the 
> future, and that it's less clear how to best use l_buf.
> 

I pretty much concur here, I don't like this part either.  I'm not quite sure 
whether we should even support buffers allocated on the stack.  That seems weird 
to me due to memory ownership issues and things potentially going out of scope. 
Something that is on the heap and reference counted would seem to be a safer 
approach.

But I'm still trying to organize my thoughts on this RFC, so I don't have 
anything I can express eloquently.

Regards,
-Denis

Re: [PATCH 1/3] buf: Add a generic extensible buffer struct

[Denis Kenzior]

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

Hi Andrew,

> Right, most real "classes" in ell are opaque in this sense.  I felt
> that the low level nature of this utility and the fact that it
> replaces basic (c builtin) mechanisms guarantees it being just a
> struct, like the skbufs in the kernel.

skbuf is a bit strange because it has public and private members.  And then some 
things are expected to be done via the API and some via accessing members. 
There are probably very good reasons for all that, but I'm not sure we want to 
use that as a model in ell.

> 
> I don't know how much effect inlining functions really has, even in
> hot paths.  The other
> feature here that uses the struct definition is that we allow the
> nested l_bufs to be allocated on stack.  For example:
> 
> struct l_buf *attr_buf = l_genl_msg_append_attr_buf(msg,
> NL80211_ATTR_IE, 100, 100);
> 
> if (is_rsn) {
>    struct ie_rsn_info rsn = { ... };
>    struct l_buf rsn_buf; /* On stack */
> 
>    l_buf_append_init(&rsn_buf, attr_buf, 50, 50);
>    ie_build_rnse(&rsn_buf, &rsn);

So, are you focusing on the ATTR_IE building as one of the use cases for this? 
With ATTR_IE we generally build the RSN and other IEs well before the genl 
message exists though?  So by the time we can call  l_genl_msg_append_attr_buf, 
the IE is already inside the handshake object or in a scratch buffer and we have 
to memcpy it anyway.

I guess you have some of that in ap.c.  But then, RSNE is typically like 40 
bytes, so not sure this is worth it to try and optimize a memcpy for it?

>    /* This is made up.  We can have ie_build_rsne allocate rsn_buf
> internally.  It could also append directly to attr_buf but in that
> case it can't *prepend* data and has to build it sequentially. */
> }

Do you have a use case in mind for prepending?

It might be nice to take some part of ap.c or p2p.c from iwd and try converting 
it so we can see how things would look like on a concrete example.

Regards,
-Denis

Re: [PATCH 1/3] buf: Add a generic extensible buffer struct

[Andrew Zaborowski]

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

Hi,

On Fri, 22 Jan 2021 at 06:43, Denis Kenzior <denkenz@gmail.com> wrote:
> > Right, most real "classes" in ell are opaque in this sense.  I felt
> > that the low level nature of this utility and the fact that it
> > replaces basic (c builtin) mechanisms guarantees it being just a
> > struct, like the skbufs in the kernel.
>
> skbuf is a bit strange because it has public and private members.  And then some
> things are expected to be done via the API and some via accessing members.
> There are probably very good reasons for all that, but I'm not sure we want to
> use that as a model in ell.
>
> >
> > I don't know how much effect inlining functions really has, even in
> > hot paths.  The other
> > feature here that uses the struct definition is that we allow the
> > nested l_bufs to be allocated on stack.  For example:
> >
> > struct l_buf *attr_buf = l_genl_msg_append_attr_buf(msg,
> > NL80211_ATTR_IE, 100, 100);
> >
> > if (is_rsn) {
> >    struct ie_rsn_info rsn = { ... };
> >    struct l_buf rsn_buf; /* On stack */
> >
> >    l_buf_append_init(&rsn_buf, attr_buf, 50, 50);
> >    ie_build_rnse(&rsn_buf, &rsn);
>
> So, are you focusing on the ATTR_IE building as one of the use cases for this?
> With ATTR_IE we generally build the RSN and other IEs well before the genl
> message exists though?  So by the time we can call  l_genl_msg_append_attr_buf,
> the IE is already inside the handshake object or in a scratch buffer and we have
> to memcpy it anyway.
>
> I guess you have some of that in ap.c.  But then, RSNE is typically like 40
> bytes, so not sure this is worth it to try and optimize a memcpy for it?

Right, I think we don't have any good examples where we'd be passing a
lot of data or have many nested structures.  I took building IE
sequences as an example, I think for now this is the only potential
user (outside of ell) of the l_buf API since the rest of the changes
can be hidden by l_genl.

>
> >    /* This is made up.  We can have ie_build_rsne allocate rsn_buf
> > internally.  It could also append directly to attr_buf but in that
> > case it can't *prepend* data and has to build it sequentially. */
> > }
>
> Do you have a use case in mind for prepending?

So this whole special buffer idea is most useful when you have
multiple layers of nested messages like in the kernel network stack,
our use cases are kinda trivial in comparison.  I have 3
l_buf_prepend() calls in the genl patch, the use case is something
like the following:

* P2P wants to update the P2P IE in the beacons, it calls ap.c API to
get an l_buf

* ap.c knows how much headroom and tailroom it needs for the other
beacon elements, it calls l_genl to create a message and get an
attribute l_buf like in the example in my reply to Mat,

* l_genl knows how much headroom and tailroom it needs for the
attribute, it creates the nested attribute l_buf from the "base"
message l_buf and returns it,

* ap.c just returns it to P2P.

* P2P writes its data (P2P IE, WFD IE) probably using the append API
but then it calls back into ap.c

* Now in ap.c since the IE order doesn't matter (I believe?) we can
prepend or append the other IEs, it doesn't matter.  Or we can create
nested l_bufs for each IE, we have various options here.

* l_genl prepends the attribute headers and the message headers and
sends the message.

So hmm, basically l_genl is the only user that benefits from the
prepend calls in this particular scenario.  I think internally in P2P,
WFD can be seen as aonther layer on top of P2P but since the P2P IE
and WSC IE don't share any header, there's no use case for
l_buf_prepend.

The situation is going to look very similar with frame-xchg.  When
p2p.c wants to send the P2P Negotiation Request frame it gets an l_buf
from frame-xchg.c instead of from ap.c, but frame-xchg.c doesn't need
to prepend any extra headers to the frame data so technically there's
no use case for l_buf_prepend either.

Eventually we could integrate ie_tlv_encapsulate_wsc_payload and
ie_tlv_encapsulate_p2p_payload into wsc_attr_builder and
p2p_attr_builder to avoid more memcpy's and they could also use l_buf.
When we want to build a new P2P IE for the beacon or another frame,
we'd first call ap.c/frame-xchg.c to get an l_buf and we'd then pass
it to p2p_build_{frame type}.  That function has no chance to add its
headroom beacuse it's not allocating the l_buf, but it can nested
l_bufs if there's any IE subelement that is big or complicated enough,
and then it'd use l_buf_prepend() for the attribute header.  That said
I don't think it's worth it for any of the subelements we currently
use.

>
> It might be nice to take some part of ap.c or p2p.c from iwd and try converting
> it so we can see how things would look like on a concrete example.

In ap.c I think the beacon update API is going to be the main explicit
user.  We don't have that API yet because it's blocked by the l_buf
stuff ;-)  But I imagine it as two functions something like:

struct l_buf *ap_new_beacon(struct ap_state *ap, size_t headroom,
size_t tailroom)
{
    struct l_genl_msg *cmd;
    uint64_t wdev_id = netdev_get_wdev_id(ap->netdev);
    struct l_buf *buf;
    static const uint8_t bcast_addr[6] = {
        0xff, 0xff, 0xff, 0xff, 0xff, 0xff
    };

    if (L_WARN_ON(!ap->started))
        return;

    cmd = l_genl_msg_new_sized(NL80211_CMD_SET_BEACON, 32);
    l_genl_msg_append_attr(cmd, NL80211_ATTR_IE, 0, "");
    l_genl_msg_append_attr(cmd, NL80211_ATTR_IE_PROBE_RESP, 0, "");
    l_genl_msg_append_attr(cmd, NL80211_ATTR_IE_ASSOC_RESP, 0, "");

    buf = l_genl_msg_append_attr_buf(cmd, NL80211_ATTR_BEACON_HEAD, 0, 256);
    ap_build_beacon_pr_head(ap, MPDU_MANAGEMENT_SUBTYPE_BEACON,
bcast_addr, buf);

    buf = l_genl_msg_append_attr_buf(cmd, NL80211_ATTR_BEACON_HEAD, 0, 256);
    ap_build_beacon_pr_head(ap, MPDU_MANAGEMENT_SUBTYPE_BEACON,
bcast_addr, buf);

    ap->pending_set_beacon = cmd;
    return buf;
}

void ap_new_beacon_done(struct ap_state *ap)
{
    if (l_genl_family_send(ap->nl80211, ap->pending_set_beacon,
ap_set_beacon_cb, NULL, NULL))
        goto done;

    l_genl_msg_unref(ap->pending_set_beacon);
    l_error("Issuing SET_BEACON failed");
done:
    ap->pending_set_beacon = NULL;
}

Alternatively, since the P2P IE also needs to be added to all other
frame types (association, probe response, etc.), the beacon IEs can
use the same mechanism as I propsed in:
https://lists.01.org/hyperkitty/list/iwd(a)lists.01.org/thread/MCYQTO54DH3GOSFMTVRVUIIORPICFL7K/
So basically p2p.c calls ap_update_beacon(ap) without passing any
contents.  This calls
ap->ops->write_mgmt_frame_ies(MPDU_MANAGEMENT_SUBTYPE_BEACON, buf,
ap->user_data) and p2p.c synchronously writes the contents into the
provided l_buf.  It has no chance to give the buffer size to
preallocate but the buffer is extensible.

Best regards

Re: [PATCH 1/3] buf: Add a generic extensible buffer struct

[Andrew Zaborowski]

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

Hi Denis and Mat,

On Fri, 22 Jan 2021 at 03:44, Denis Kenzior <denkenz@gmail.com> wrote:
> > Thanks for the additional background, and for letting me know it's still RFC-stage.
> >
> > I think what it comes down to for me is that exposing the struct internals locks
> > those in as part of the "public API" that would be harder to change in the
> > future, and that it's less clear how to best use l_buf.
> >
>
> I pretty much concur here, I don't like this part either.  I'm not quite sure
> whether we should even support buffers allocated on the stack.  That seems weird
> to me due to memory ownership issues and things potentially going out of scope.

Right, it's a little tricky but I think I came up with semantics (and
implementation) that work.  In any case I'm fine dropping the static
l_bufs thing and not exposing the struct declaration.

Best regards

Re: [PATCH 1/3] buf: Add a generic extensible buffer struct

[Andrew Zaborowski]

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

On Fri, 22 Jan 2021 at 12:05, Andrew Zaborowski
<andrew.zaborowski@intel.com> wrote:
> On Fri, 22 Jan 2021 at 06:43, Denis Kenzior <denkenz@gmail.com> wrote:
> > > Right, most real "classes" in ell are opaque in this sense.  I felt
> > > that the low level nature of this utility and the fact that it
> > > replaces basic (c builtin) mechanisms guarantees it being just a
> > > struct, like the skbufs in the kernel.
> >
> > skbuf is a bit strange because it has public and private members.  And then some
> > things are expected to be done via the API and some via accessing members.
> > There are probably very good reasons for all that, but I'm not sure we want to
> > use that as a model in ell.
> >
> > >
> > > I don't know how much effect inlining functions really has, even in
> > > hot paths.  The other
> > > feature here that uses the struct definition is that we allow the
> > > nested l_bufs to be allocated on stack.  For example:
> > >
> > > struct l_buf *attr_buf = l_genl_msg_append_attr_buf(msg,
> > > NL80211_ATTR_IE, 100, 100);
> > >
> > > if (is_rsn) {
> > >    struct ie_rsn_info rsn = { ... };
> > >    struct l_buf rsn_buf; /* On stack */
> > >
> > >    l_buf_append_init(&rsn_buf, attr_buf, 50, 50);
> > >    ie_build_rnse(&rsn_buf, &rsn);
> >
> > So, are you focusing on the ATTR_IE building as one of the use cases for this?
> > With ATTR_IE we generally build the RSN and other IEs well before the genl
> > message exists though?  So by the time we can call  l_genl_msg_append_attr_buf,
> > the IE is already inside the handshake object or in a scratch buffer and we have
> > to memcpy it anyway.
> >
> > I guess you have some of that in ap.c.  But then, RSNE is typically like 40
> > bytes, so not sure this is worth it to try and optimize a memcpy for it?
>
> Right, I think we don't have any good examples where we'd be passing a
> lot of data or have many nested structures.  I took building IE
> sequences as an example, I think for now this is the only potential
> user (outside of ell) of the l_buf API since the rest of the changes
> can be hidden by l_genl.
>
> >
> > >    /* This is made up.  We can have ie_build_rsne allocate rsn_buf
> > > internally.  It could also append directly to attr_buf but in that
> > > case it can't *prepend* data and has to build it sequentially. */
> > > }
> >
> > Do you have a use case in mind for prepending?
>
> So this whole special buffer idea is most useful when you have
> multiple layers of nested messages like in the kernel network stack,
> our use cases are kinda trivial in comparison.  I have 3
> l_buf_prepend() calls in the genl patch, the use case is something
> like the following:
>
> * P2P wants to update the P2P IE in the beacons, it calls ap.c API to
> get an l_buf
>
> * ap.c knows how much headroom and tailroom it needs for the other
> beacon elements, it calls l_genl to create a message and get an
> attribute l_buf like in the example in my reply to Mat,
>
> * l_genl knows how much headroom and tailroom it needs for the
> attribute, it creates the nested attribute l_buf from the "base"
> message l_buf and returns it,
>
> * ap.c just returns it to P2P.
>
> * P2P writes its data (P2P IE, WFD IE) probably using the append API
> but then it calls back into ap.c
>
> * Now in ap.c since the IE order doesn't matter (I believe?) we can
> prepend or append the other IEs, it doesn't matter.  Or we can create
> nested l_bufs for each IE, we have various options here.
>
> * l_genl prepends the attribute headers and the message headers and
> sends the message.
>
> So hmm, basically l_genl is the only user that benefits from the
> prepend calls in this particular scenario.  I think internally in P2P,
> WFD can be seen as aonther layer on top of P2P but since the P2P IE
> and WSC IE don't share any header, there's no use case for
> l_buf_prepend.
>
> The situation is going to look very similar with frame-xchg.  When
> p2p.c wants to send the P2P Negotiation Request frame it gets an l_buf
> from frame-xchg.c instead of from ap.c, but frame-xchg.c doesn't need
> to prepend any extra headers to the frame data so technically there's
> no use case for l_buf_prepend either.
>
> Eventually we could integrate ie_tlv_encapsulate_wsc_payload and
> ie_tlv_encapsulate_p2p_payload into wsc_attr_builder and
> p2p_attr_builder to avoid more memcpy's and they could also use l_buf.
> When we want to build a new P2P IE for the beacon or another frame,
> we'd first call ap.c/frame-xchg.c to get an l_buf and we'd then pass
> it to p2p_build_{frame type}.  That function has no chance to add its
> headroom beacuse it's not allocating the l_buf, but it can nested
> l_bufs if there's any IE subelement that is big or complicated enough,
> and then it'd use l_buf_prepend() for the attribute header.  That said
> I don't think it's worth it for any of the subelements we currently
> use.
>
> >
> > It might be nice to take some part of ap.c or p2p.c from iwd and try converting
> > it so we can see how things would look like on a concrete example.
>
> In ap.c I think the beacon update API is going to be the main explicit
> user.  We don't have that API yet because it's blocked by the l_buf
> stuff ;-)  But I imagine it as two functions something like:
>
> struct l_buf *ap_new_beacon(struct ap_state *ap, size_t headroom,
> size_t tailroom)
> {
>     struct l_genl_msg *cmd;
>     uint64_t wdev_id = netdev_get_wdev_id(ap->netdev);
>     struct l_buf *buf;
>     static const uint8_t bcast_addr[6] = {
>         0xff, 0xff, 0xff, 0xff, 0xff, 0xff
>     };
>
>     if (L_WARN_ON(!ap->started))
>         return;
>
>     cmd = l_genl_msg_new_sized(NL80211_CMD_SET_BEACON, 32);
>     l_genl_msg_append_attr(cmd, NL80211_ATTR_IE, 0, "");
>     l_genl_msg_append_attr(cmd, NL80211_ATTR_IE_PROBE_RESP, 0, "");
>     l_genl_msg_append_attr(cmd, NL80211_ATTR_IE_ASSOC_RESP, 0, "");
>
>     buf = l_genl_msg_append_attr_buf(cmd, NL80211_ATTR_BEACON_HEAD, 0, 256);
>     ap_build_beacon_pr_head(ap, MPDU_MANAGEMENT_SUBTYPE_BEACON,
> bcast_addr, buf);
>
>     buf = l_genl_msg_append_attr_buf(cmd, NL80211_ATTR_BEACON_HEAD, 0, 256);
>     ap_build_beacon_pr_head(ap, MPDU_MANAGEMENT_SUBTYPE_BEACON,
> bcast_addr, buf);

This was supposed to read:
     buf = l_genl_msg_append_attr_buf(cmd, NL80211_ATTR_BEACON_TAIL, 0, 256);
     ap_build_beacon_pr_tail(ap, MPDU_MANAGEMENT_SUBTYPE_BEACON,
bcast_addr, buf);

>
>     ap->pending_set_beacon = cmd;
>     return buf;

And this would need to be:
    return l_buf_append_new(buf, headroom, tailroom);

> }

Best regards

Re: [PATCH 1/3] buf: Add a generic extensible buffer struct

[Denis Kenzior]

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

Hi Andrew,

> Right, I think we don't have any good examples where we'd be passing a
> lot of data or have many nested structures.  I took building IE
> sequences as an example, I think for now this is the only potential
> user (outside of ell) of the l_buf API since the rest of the changes
> can be hidden by l_genl.

Hmm, what about EAP-TLS? Do we have use cases there?

> 
>>
>>>     /* This is made up.  We can have ie_build_rsne allocate rsn_buf
>>> internally.  It could also append directly to attr_buf but in that
>>> case it can't *prepend* data and has to build it sequentially. */
>>> }
>>
>> Do you have a use case in mind for prepending?
> 
> So this whole special buffer idea is most useful when you have
> multiple layers of nested messages like in the kernel network stack,

Right, this would be useful in EAP for example where we manage the headroom by 
hand...

> our use cases are kinda trivial in comparison.  I have 3
> l_buf_prepend() calls in the genl patch, the use case is something
> like the following:
> 
> * P2P wants to update the P2P IE in the beacons, it calls ap.c API to
> get an l_buf
> 
> * ap.c knows how much headroom and tailroom it needs for the other
> beacon elements, it calls l_genl to create a message and get an
> attribute l_buf like in the example in my reply to Mat,
> 
> * l_genl knows how much headroom and tailroom it needs for the
> attribute, it creates the nested attribute l_buf from the "base"
> message l_buf and returns it,
> 
> * ap.c just returns it to P2P.
> 
> * P2P writes its data (P2P IE, WFD IE) probably using the append API
> but then it calls back into ap.c
> 
> * Now in ap.c since the IE order doesn't matter (I believe?) we can
> prepend or append the other IEs, it doesn't matter.  Or we can create
> nested l_bufs for each IE, we have various options here.

Actually, the order does matter.  Each frame type specifies the order.

802.11-2016, Section 9.4.2.1:
"The frame body components specified for many management subtypes result in 
elements ordered by ascending values of the Element ID field and then the 
Element ID Extension field (when present), with the exception of the MIC 
Management element (9.4.2.55). If present, the MIC Management element appears at 
the end of the robust management frame body. See 10.27.6 on the parsing of 
elements."

> 
> * l_genl prepends the attribute headers and the message headers and
> sends the message.
> 
> So hmm, basically l_genl is the only user that benefits from the
> prepend calls in this particular scenario.  I think internally in P2P,
> WFD can be seen as aonther layer on top of P2P but since the P2P IE
> and WSC IE don't share any header, there's no use case for
> l_buf_prepend.
> 
> The situation is going to look very similar with frame-xchg.  When
> p2p.c wants to send the P2P Negotiation Request frame it gets an l_buf
> from frame-xchg.c instead of from ap.c, but frame-xchg.c doesn't need
> to prepend any extra headers to the frame data so technically there's
> no use case for l_buf_prepend either.

So can we get away with a simplified implementation for headroom management? 
Like what the kernel skbuf does basically.  You create the buffer and 
immediately set the headroom, which just moves the pointer.  Once data has been 
added, headroom can't be extended without making a copy of the buffer.   This 
generally works out well because the message specific headroom is known ahead of 
time by some means (perhaps by a driver setting it when registering, etc)

> 
> Eventually we could integrate ie_tlv_encapsulate_wsc_payload and
> ie_tlv_encapsulate_p2p_payload into wsc_attr_builder and
> p2p_attr_builder to avoid more memcpy's and they could also use l_buf.
> When we want to build a new P2P IE for the beacon or another frame,
> we'd first call ap.c/frame-xchg.c to get an l_buf and we'd then pass
> it to p2p_build_{frame type}.  That function has no chance to add its
> headroom beacuse it's not allocating the l_buf, but it can nested
> l_bufs if there's any IE subelement that is big or complicated enough,
> and then it'd use l_buf_prepend() for the attribute header.  That said
> I don't think it's worth it for any of the subelements we currently
> use.

In the case of WSC at least it may be easier to just specify how we want the 
element to be built.  So we can fragment it as we build it for example.

> 
>>
>> It might be nice to take some part of ap.c or p2p.c from iwd and try converting
>> it so we can see how things would look like on a concrete example.
> 
> In ap.c I think the beacon update API is going to be the main explicit
> user.  We don't have that API yet because it's blocked by the l_buf
> stuff ;-)  But I imagine it as two functions something like:
> 
> struct l_buf *ap_new_beacon(struct ap_state *ap, size_t headroom,
> size_t tailroom)
> {
>      struct l_genl_msg *cmd;
>      uint64_t wdev_id = netdev_get_wdev_id(ap->netdev);
>      struct l_buf *buf;
>      static const uint8_t bcast_addr[6] = {
>          0xff, 0xff, 0xff, 0xff, 0xff, 0xff
>      };
> 
>      if (L_WARN_ON(!ap->started))
>          return;
> 
>      cmd = l_genl_msg_new_sized(NL80211_CMD_SET_BEACON, 32);
>      l_genl_msg_append_attr(cmd, NL80211_ATTR_IE, 0, "");
>      l_genl_msg_append_attr(cmd, NL80211_ATTR_IE_PROBE_RESP, 0, "");
>      l_genl_msg_append_attr(cmd, NL80211_ATTR_IE_ASSOC_RESP, 0, "");
> 
>      buf = l_genl_msg_append_attr_buf(cmd, NL80211_ATTR_BEACON_HEAD, 0, 256);
>      ap_build_beacon_pr_head(ap, MPDU_MANAGEMENT_SUBTYPE_BEACON,
> bcast_addr, buf);
> 
>      buf = l_genl_msg_append_attr_buf(cmd, NL80211_ATTR_BEACON_HEAD, 0, 256);
>      ap_build_beacon_pr_head(ap, MPDU_MANAGEMENT_SUBTYPE_BEACON,
> bcast_addr, buf);
> 
>      ap->pending_set_beacon = cmd;
>      return buf;
> }
> 
> void ap_new_beacon_done(struct ap_state *ap)
> {
>      if (l_genl_family_send(ap->nl80211, ap->pending_set_beacon,
> ap_set_beacon_cb, NULL, NULL))
>          goto done;
> 
>      l_genl_msg_unref(ap->pending_set_beacon);
>      l_error("Issuing SET_BEACON failed");
> done:
>      ap->pending_set_beacon = NULL;
> }
> 
> Alternatively, since the P2P IE also needs to be added to all other
> frame types (association, probe response, etc.), the beacon IEs can
> use the same mechanism as I propsed in:
> https://lists.01.org/hyperkitty/list/iwd(a)lists.01.org/thread/MCYQTO54DH3GOSFMTVRVUIIORPICFL7K/
> So basically p2p.c calls ap_update_beacon(ap) without passing any
> contents.  This calls
> ap->ops->write_mgmt_frame_ies(MPDU_MANAGEMENT_SUBTYPE_BEACON, buf,
> ap->user_data) and p2p.c synchronously writes the contents into the
> provided l_buf.  It has no chance to give the buffer size to
> preallocate but the buffer is extensible.
> 

Hmm, ok.  Let me re-examine this code path again.  It almost feels like we can 
fill-out and save for future use much of the beacon head / tails and not build 
them every time.

Regards,
-Denis

Re: [PATCH 1/3] buf: Add a generic extensible buffer struct

[Andrew Zaborowski]

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

On Fri, 22 Jan 2021 at 17:07, Denis Kenzior <denkenz@gmail.com> wrote:
> > Right, I think we don't have any good examples where we'd be passing a
> > lot of data or have many nested structures.  I took building IE
> > sequences as an example, I think for now this is the only potential
> > user (outside of ell) of the l_buf API since the rest of the changes
> > can be hidden by l_genl.
>
> Hmm, what about EAP-TLS? Do we have use cases there?
>
> >
> >>
> >>>     /* This is made up.  We can have ie_build_rsne allocate rsn_buf
> >>> internally.  It could also append directly to attr_buf but in that
> >>> case it can't *prepend* data and has to build it sequentially. */
> >>> }
> >>
> >> Do you have a use case in mind for prepending?
> >
> > So this whole special buffer idea is most useful when you have
> > multiple layers of nested messages like in the kernel network stack,
>
> Right, this would be useful in EAP for example where we manage the headroom by
> hand...

True, EAP has a lot of layers especially with phase 2.  We don't have
big messages where it would make a difference.  But we could convert
all the methods, and TLS, and perhaps the EAP headers would be
prepended using l_buf_prepend.  Within TLS alone I don't think we're
gaining much.

>
> > our use cases are kinda trivial in comparison.  I have 3
> > l_buf_prepend() calls in the genl patch, the use case is something
> > like the following:
> >
> > * P2P wants to update the P2P IE in the beacons, it calls ap.c API to
> > get an l_buf
> >
> > * ap.c knows how much headroom and tailroom it needs for the other
> > beacon elements, it calls l_genl to create a message and get an
> > attribute l_buf like in the example in my reply to Mat,
> >
> > * l_genl knows how much headroom and tailroom it needs for the
> > attribute, it creates the nested attribute l_buf from the "base"
> > message l_buf and returns it,
> >
> > * ap.c just returns it to P2P.
> >
> > * P2P writes its data (P2P IE, WFD IE) probably using the append API
> > but then it calls back into ap.c
> >
> > * Now in ap.c since the IE order doesn't matter (I believe?) we can
> > prepend or append the other IEs, it doesn't matter.  Or we can create
> > nested l_bufs for each IE, we have various options here.
>
> Actually, the order does matter.  Each frame type specifies the order.
>
> 802.11-2016, Section 9.4.2.1:
> "The frame body components specified for many management subtypes result in
> elements ordered by ascending values of the Element ID field and then the
> Element ID Extension field (when present), with the exception of the MIC
> Management element (9.4.2.55). If present, the MIC Management element appears at
> the end of the robust management frame body. See 10.27.6 on the parsing of
> elements."

Ok, good to know.  If we want to follow this strictly, perhaps in
addition to the ops->write_mgmt_frame_ies callback, the user (P2P in
our case) needs to provide the IDs of the IEs it's going to add.  ap.c
would then make sure the user IEs are placed correctly.

>
> >
> > * l_genl prepends the attribute headers and the message headers and
> > sends the message.
> >
> > So hmm, basically l_genl is the only user that benefits from the
> > prepend calls in this particular scenario.  I think internally in P2P,
> > WFD can be seen as aonther layer on top of P2P but since the P2P IE
> > and WSC IE don't share any header, there's no use case for
> > l_buf_prepend.
> >
> > The situation is going to look very similar with frame-xchg.  When
> > p2p.c wants to send the P2P Negotiation Request frame it gets an l_buf
> > from frame-xchg.c instead of from ap.c, but frame-xchg.c doesn't need
> > to prepend any extra headers to the frame data so technically there's
> > no use case for l_buf_prepend either.
>
> So can we get away with a simplified implementation for headroom management?
> Like what the kernel skbuf does basically.  You create the buffer and
> immediately set the headroom, which just moves the pointer.

That's what I did, except...

> Once data has been
> added, headroom can't be extended without making a copy of the buffer.

I extend the buffer when we run out of space both when prepending and appending.

We can drop this mechanism but really it'd save us just a few lines in
the implementation (not in the API).  In fact it makes the API more
complicated because now you need to check for errors when calling
l_buf_prepend (but not l_buf_append), in the end I don't think it's a
simplification.

> This
> generally works out well because the message specific headroom is known ahead of
> time by some means (perhaps by a driver setting it when registering, etc)
>

Right, optimally the user doesn't rely on the buffer auto-extending.
But I think your earlier argument for growing the buffer at the tail,
for robustness etc., applies here too.  And it's easier because the
method always succeeds.

> >
> > Eventually we could integrate ie_tlv_encapsulate_wsc_payload and
> > ie_tlv_encapsulate_p2p_payload into wsc_attr_builder and
> > p2p_attr_builder to avoid more memcpy's and they could also use l_buf.
> > When we want to build a new P2P IE for the beacon or another frame,
> > we'd first call ap.c/frame-xchg.c to get an l_buf and we'd then pass
> > it to p2p_build_{frame type}.  That function has no chance to add its
> > headroom beacuse it's not allocating the l_buf, but it can nested
> > l_bufs if there's any IE subelement that is big or complicated enough,
> > and then it'd use l_buf_prepend() for the attribute header.  That said
> > I don't think it's worth it for any of the subelements we currently
> > use.
>
> In the case of WSC at least it may be easier to just specify how we want the
> element to be built.  So we can fragment it as we build it for example.

Sorry I didn't get you.  We can fragment the WSC IEs and P2P IEs as we
add subelements to them, transparently.

What I mean though is that we can currently get away with methods like
p2p_build_u{8,16,32}_attr and similar, and we have no subelements
complex enough to use a method that returns a nested l_buf where the
caller fills in their own data or passes the l_buf around to
functions.  I.e. no method like my l_genl_msg_append_attr_buf() for
big genl attributes.  Which means we can probably get away without
using l_buf_prepend().

>
> >
> >>
> >> It might be nice to take some part of ap.c or p2p.c from iwd and try converting
> >> it so we can see how things would look like on a concrete example.
> >
> > In ap.c I think the beacon update API is going to be the main explicit
> > user.  We don't have that API yet because it's blocked by the l_buf
> > stuff ;-)  But I imagine it as two functions something like:
> >
> > struct l_buf *ap_new_beacon(struct ap_state *ap, size_t headroom,
> > size_t tailroom)
> > {
> >      struct l_genl_msg *cmd;
> >      uint64_t wdev_id = netdev_get_wdev_id(ap->netdev);
> >      struct l_buf *buf;
> >      static const uint8_t bcast_addr[6] = {
> >          0xff, 0xff, 0xff, 0xff, 0xff, 0xff
> >      };
> >
> >      if (L_WARN_ON(!ap->started))
> >          return;
> >
> >      cmd = l_genl_msg_new_sized(NL80211_CMD_SET_BEACON, 32);
> >      l_genl_msg_append_attr(cmd, NL80211_ATTR_IE, 0, "");
> >      l_genl_msg_append_attr(cmd, NL80211_ATTR_IE_PROBE_RESP, 0, "");
> >      l_genl_msg_append_attr(cmd, NL80211_ATTR_IE_ASSOC_RESP, 0, "");
> >
> >      buf = l_genl_msg_append_attr_buf(cmd, NL80211_ATTR_BEACON_HEAD, 0, 256);
> >      ap_build_beacon_pr_head(ap, MPDU_MANAGEMENT_SUBTYPE_BEACON,
> > bcast_addr, buf);
> >
> >      buf = l_genl_msg_append_attr_buf(cmd, NL80211_ATTR_BEACON_HEAD, 0, 256);
> >      ap_build_beacon_pr_head(ap, MPDU_MANAGEMENT_SUBTYPE_BEACON,
> > bcast_addr, buf);
> >
> >      ap->pending_set_beacon = cmd;
> >      return buf;
> > }
> >
> > void ap_new_beacon_done(struct ap_state *ap)
> > {
> >      if (l_genl_family_send(ap->nl80211, ap->pending_set_beacon,
> > ap_set_beacon_cb, NULL, NULL))
> >          goto done;
> >
> >      l_genl_msg_unref(ap->pending_set_beacon);
> >      l_error("Issuing SET_BEACON failed");
> > done:
> >      ap->pending_set_beacon = NULL;
> > }
> >
> > Alternatively, since the P2P IE also needs to be added to all other
> > frame types (association, probe response, etc.), the beacon IEs can
> > use the same mechanism as I propsed in:
> > https://lists.01.org/hyperkitty/list/iwd(a)lists.01.org/thread/MCYQTO54DH3GOSFMTVRVUIIORPICFL7K/
> > So basically p2p.c calls ap_update_beacon(ap) without passing any
> > contents.  This calls
> > ap->ops->write_mgmt_frame_ies(MPDU_MANAGEMENT_SUBTYPE_BEACON, buf,
> > ap->user_data) and p2p.c synchronously writes the contents into the
> > provided l_buf.  It has no chance to give the buffer size to
> > preallocate but the buffer is extensible.
> >
>
> Hmm, ok.  Let me re-examine this code path again.  It almost feels like we can
> fill-out and save for future use much of the beacon head / tails and not build
> them every time.

I think we can, but we don't update the beacon IEs often.

Best regards