[PATCH v2 0/4] Improve genl with a message builder

[PATCH v2 0/4] Improve genl with a message builder

[Tomasz Bursztyka]

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

Hi,

It took a bit long as I was messing up with the wrong dump of a message
(a private vs a broadcasted one...) and of course it took a while before
I realized it.

Anyway, seems to work. I only had an issue with the nested message length
calculation when leaving it.

Tomasz Bursztyka (4):
  genl: Normilize functions exports and parameters check as it should
  genl: Add a message builder API to help creating complex nl messages
  genl: Add a function to grab the message's raw data for unit purpose
  unit: Add a message builder unit test for genl

 Makefile.am        |   1 +
 ell/genl-private.h |  25 +++++
 ell/genl.c         | 284 ++++++++++++++++++++++++++++++++++++++++-------------
 ell/genl.h         |  18 ++++
 unit/test-genl.c   | 136 +++++++++++++++++++++----
 5 files changed, 379 insertions(+), 85 deletions(-)
 create mode 100644 ell/genl-private.h

-- 
2.0.5

[PATCH 1/4] genl: Normilize functions exports and parameters check as it should

[Tomasz Bursztyka]

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

Ell follows a certain way of exporting library functions and checking
parameter's validity, so let's apply these rules to genl.c
---
 ell/genl.c | 152 ++++++++++++++++++++++++++++++++++---------------------------
 1 file changed, 85 insertions(+), 67 deletions(-)

diff --git a/ell/genl.c b/ell/genl.c
index 5944da6..90c0864 100644
--- a/ell/genl.c
+++ b/ell/genl.c
@@ -489,11 +489,11 @@ static bool received_data(struct l_io *io, void *user_data)
 	return true;
 }
 
-struct l_genl *l_genl_new(int fd)
+LIB_EXPORT struct l_genl *l_genl_new(int fd)
 {
 	struct l_genl *genl;
 
-	if (fd < 0)
+	if (unlikely(fd < 0))
 		return NULL;
 
 	genl = l_new(struct l_genl, 1);
@@ -522,7 +522,7 @@ struct l_genl *l_genl_new(int fd)
 	return l_genl_ref(genl);
 }
 
-struct l_genl *l_genl_new_default(void)
+LIB_EXPORT struct l_genl *l_genl_new_default(void)
 {
 	struct l_genl *genl;
 	struct sockaddr_nl addr;
@@ -563,9 +563,9 @@ struct l_genl *l_genl_new_default(void)
 	return genl;
 }
 
-struct l_genl *l_genl_ref(struct l_genl *genl)
+LIB_EXPORT struct l_genl *l_genl_ref(struct l_genl *genl)
 {
-	if (!genl)
+	if (unlikely(!genl))
 		return NULL;
 
 	__sync_fetch_and_add(&genl->ref_count, 1);
@@ -573,9 +573,9 @@ struct l_genl *l_genl_ref(struct l_genl *genl)
 	return genl;
 }
 
-void l_genl_unref(struct l_genl *genl)
+LIB_EXPORT void l_genl_unref(struct l_genl *genl)
 {
-	if (!genl)
+	if (unlikely(!genl))
 		return;
 
 	if (__sync_sub_and_fetch(&genl->ref_count, 1))
@@ -604,10 +604,12 @@ void l_genl_unref(struct l_genl *genl)
 	l_free(genl);
 }
 
-bool l_genl_set_debug(struct l_genl *genl, l_genl_debug_func_t callback,
-				void *user_data, l_genl_destroy_func_t destroy)
+LIB_EXPORT bool l_genl_set_debug(struct l_genl *genl,
+					l_genl_debug_func_t callback,
+					void *user_data,
+					l_genl_destroy_func_t destroy)
 {
-	if (!genl)
+	if (unlikely(!genl))
 		return false;
 
 	if (genl->debug_destroy)
@@ -620,9 +622,9 @@ bool l_genl_set_debug(struct l_genl *genl, l_genl_debug_func_t callback,
 	return true;
 }
 
-bool l_genl_set_close_on_unref(struct l_genl *genl, bool do_close)
+LIB_EXPORT bool l_genl_set_close_on_unref(struct l_genl *genl, bool do_close)
 {
-	if (!genl)
+	if (unlikely(!genl))
 		return false;
 
 	genl->close_on_unref = do_close;
@@ -630,19 +632,19 @@ bool l_genl_set_close_on_unref(struct l_genl *genl, bool do_close)
 	return true;
 }
 
-struct l_genl_msg *l_genl_msg_new(uint8_t cmd)
+LIB_EXPORT struct l_genl_msg *l_genl_msg_new(uint8_t cmd)
 {
 	return l_genl_msg_new_sized(cmd, 0);
 }
 
-struct l_genl_msg *l_genl_msg_new_sized(uint8_t cmd, uint32_t size)
+LIB_EXPORT struct l_genl_msg *l_genl_msg_new_sized(uint8_t cmd, uint32_t size)
 {
 	return msg_alloc(cmd, 0x00, size);
 }
 
-struct l_genl_msg *l_genl_msg_ref(struct l_genl_msg *msg)
+LIB_EXPORT struct l_genl_msg *l_genl_msg_ref(struct l_genl_msg *msg)
 {
-	if (!msg)
+	if (unlikely(!msg))
 		return NULL;
 
 	__sync_fetch_and_add(&msg->ref_count, 1);
@@ -650,9 +652,9 @@ struct l_genl_msg *l_genl_msg_ref(struct l_genl_msg *msg)
 	return msg;
 }
 
-void l_genl_msg_unref(struct l_genl_msg *msg)
+LIB_EXPORT void l_genl_msg_unref(struct l_genl_msg *msg)
 {
-	if (!msg)
+	if (unlikely(!msg))
 		return;
 
 	if (__sync_sub_and_fetch(&msg->ref_count, 1))
@@ -662,36 +664,36 @@ void l_genl_msg_unref(struct l_genl_msg *msg)
 	l_free(msg);
 }
 
-uint8_t l_genl_msg_get_command(struct l_genl_msg *msg)
+LIB_EXPORT uint8_t l_genl_msg_get_command(struct l_genl_msg *msg)
 {
-	if (!msg)
+	if (unlikely(!msg))
 		return 0;
 
 	return msg->cmd;
 }
 
-uint8_t l_genl_msg_get_version(struct l_genl_msg *msg)
+LIB_EXPORT uint8_t l_genl_msg_get_version(struct l_genl_msg *msg)
 {
-	if (!msg)
+	if (unlikely(!msg))
 		return 0;
 
 	return msg->version;
 }
 
-int l_genl_msg_get_error(struct l_genl_msg *msg)
+LIB_EXPORT int l_genl_msg_get_error(struct l_genl_msg *msg)
 {
-	if (!msg)
+	if (unlikely(!msg))
 		return -ENOMSG;
 
 	return msg->error;
 }
 
-bool l_genl_msg_append_attr(struct l_genl_msg *msg, uint16_t type,
-					uint16_t len, const void *data)
+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;
 
-	if (!msg)
+	if (unlikely(!msg))
 		return false;
 
 	if (msg->len + NLA_HDRLEN + NLA_ALIGN(len) > msg->size)
@@ -718,12 +720,13 @@ bool l_genl_msg_append_attr(struct l_genl_msg *msg, uint16_t type,
 #define NLA_DATA(nla)		((void*)(((char*)(nla)) + NLA_LENGTH(0)))
 #define NLA_PAYLOAD(nla)	((int)((nla)->nla_len) - NLA_LENGTH(0))
 
-bool l_genl_attr_init(struct l_genl_attr *attr, struct l_genl_msg *msg)
+LIB_EXPORT bool l_genl_attr_init(struct l_genl_attr *attr,
+						struct l_genl_msg *msg)
 {
 	const struct nlattr *nla;
 	uint32_t len;
 
-	if (!attr || !msg)
+	if (unlikely(!attr) || unlikely(!msg))
 		return false;
 
 	if (!msg->data || msg->len < NLMSG_HDRLEN + GENL_HDRLEN)
@@ -744,12 +747,14 @@ bool l_genl_attr_init(struct l_genl_attr *attr, struct l_genl_msg *msg)
 	return true;
 }
 
-bool l_genl_attr_next(struct l_genl_attr *attr, uint16_t *type,
-					uint16_t *len, const void **data)
+LIB_EXPORT bool l_genl_attr_next(struct l_genl_attr *attr,
+						uint16_t *type,
+						uint16_t *len,
+						const void **data)
 {
 	const struct nlattr *nla;
 
-	if (!attr)
+	if (unlikely(!attr))
 		return false;
 
 	nla = attr->next_data;
@@ -774,11 +779,12 @@ bool l_genl_attr_next(struct l_genl_attr *attr, uint16_t *type,
 	return true;
 }
 
-bool l_genl_attr_recurse(struct l_genl_attr *attr, struct l_genl_attr *nested)
+LIB_EXPORT bool l_genl_attr_recurse(struct l_genl_attr *attr,
+						struct l_genl_attr *nested)
 {
 	const struct nlattr *nla;
 
-	if (!attr || !nested)
+	if (unlikely(!attr) || unlikely(!nested))
 		return false;
 
 	nla = attr->data;
@@ -905,12 +911,13 @@ static void get_family_callback(struct l_genl_msg *msg, void *user_data)
 		family->watch_appeared(family->watch_data);
 }
 
-struct l_genl_family *l_genl_family_new(struct l_genl *genl, const char *name)
+LIB_EXPORT struct l_genl_family *l_genl_family_new(struct l_genl *genl,
+							const char *name)
 {
 	struct l_genl_family *family;
 	struct l_genl_msg *msg;
 
-	if (!genl || !name)
+	if (unlikely(!genl) || unlikely(!name))
 		return NULL;
 
 	family = family_alloc(genl, name);
@@ -935,9 +942,10 @@ struct l_genl_family *l_genl_family_new(struct l_genl *genl, const char *name)
 	return family;
 }
 
-struct l_genl_family *l_genl_family_ref(struct l_genl_family *family)
+LIB_EXPORT struct l_genl_family *l_genl_family_ref(
+						struct l_genl_family *family)
 {
-	if (!family)
+	if (unlikely(!family))
 		return NULL;
 
 	__sync_fetch_and_add(&family->ref_count, 1);
@@ -945,11 +953,11 @@ struct l_genl_family *l_genl_family_ref(struct l_genl_family *family)
 	return family;
 }
 
-void l_genl_family_unref(struct l_genl_family *family)
+LIB_EXPORT void l_genl_family_unref(struct l_genl_family *family)
 {
 	struct l_genl *genl;
 
-	if (!family)
+	if (unlikely(!family))
 		return;
 
 	if (__sync_sub_and_fetch(&family->ref_count, 1))
@@ -975,12 +983,13 @@ void l_genl_family_unref(struct l_genl_family *family)
 	l_free(family);
 }
 
-bool l_genl_family_set_watches(struct l_genl_family *family,
-				l_genl_watch_func_t appeared,
-				l_genl_watch_func_t vanished,
-				void *user_data, l_genl_destroy_func_t destroy)
+LIB_EXPORT bool l_genl_family_set_watches(struct l_genl_family *family,
+						l_genl_watch_func_t appeared,
+						l_genl_watch_func_t vanished,
+						void *user_data,
+						l_genl_destroy_func_t destroy)
 {
-	if (!family)
+	if (unlikely(!family))
 		return false;
 
 	if (family->watch_destroy)
@@ -994,9 +1003,9 @@ bool l_genl_family_set_watches(struct l_genl_family *family,
 	return true;
 }
 
-uint32_t l_genl_family_get_version(struct l_genl_family *family)
+LIB_EXPORT uint32_t l_genl_family_get_version(struct l_genl_family *family)
 {
-	if (!family)
+	if (unlikely(!family))
 		return 0;
 
 	return family->version;
@@ -1010,11 +1019,12 @@ static bool match_op_id(const void *a, const void *b)
 	return op->id == id;
 }
 
-bool l_genl_family_can_send(struct l_genl_family *family, uint8_t cmd)
+LIB_EXPORT bool l_genl_family_can_send(struct l_genl_family *family,
+								uint8_t cmd)
 {
 	struct genl_op *op;
 
-	if (!family)
+	if (unlikely(!family))
 		return false;
 
 	op = l_queue_find(family->op_list, match_op_id, L_UINT_TO_PTR(cmd));
@@ -1027,7 +1037,8 @@ bool l_genl_family_can_send(struct l_genl_family *family, uint8_t cmd)
 	return false;
 }
 
-bool l_genl_family_can_dump(struct l_genl_family *family, uint8_t cmd)
+LIB_EXPORT bool l_genl_family_can_dump(struct l_genl_family *family,
+								uint8_t cmd)
 {
 	struct genl_op *op;
 
@@ -1081,19 +1092,21 @@ static unsigned int send_common(struct l_genl_family *family, uint16_t flags,
 	return request->id;
 }
 
-unsigned int l_genl_family_send(struct l_genl_family *family,
-				struct l_genl_msg *msg,
-				l_genl_msg_func_t callback,
-				void *user_data, l_genl_destroy_func_t destroy)
+LIB_EXPORT unsigned int l_genl_family_send(struct l_genl_family *family,
+						struct l_genl_msg *msg,
+						l_genl_msg_func_t callback,
+						void *user_data,
+						l_genl_destroy_func_t destroy)
 {
 	return send_common(family, NLM_F_ACK, msg, callback,
 						user_data, destroy);
 }
 
-unsigned int l_genl_family_dump(struct l_genl_family *family,
-				struct l_genl_msg *msg,
-				l_genl_msg_func_t callback,
-				void *user_data, l_genl_destroy_func_t destroy)
+LIB_EXPORT unsigned int l_genl_family_dump(struct l_genl_family *family,
+						struct l_genl_msg *msg,
+						l_genl_msg_func_t callback,
+						void *user_data,
+						l_genl_destroy_func_t destroy)
 {
 	return send_common(family, NLM_F_ACK | NLM_F_DUMP, msg, callback,
 							user_data, destroy);
@@ -1107,12 +1120,13 @@ static bool match_request_id(const void *a, const void *b)
 	return request->id == id;
 }
 
-bool l_genl_family_cancel(struct l_genl_family *family, unsigned int id)
+LIB_EXPORT bool l_genl_family_cancel(struct l_genl_family *family,
+							unsigned int id)
 {
 	struct l_genl *genl;
 	struct genl_request *request;
 
-	if (!family || !id)
+	if (unlikely(!family) || unlikely(!id))
 		return false;
 
 	genl = family->genl;
@@ -1149,11 +1163,12 @@ static void add_membership(struct l_genl *genl, struct genl_mcast *mcast)
 	mcast->users++;
 }
 
-bool l_genl_family_has_group(struct l_genl_family *family, const char *group)
+LIB_EXPORT bool l_genl_family_has_group(struct l_genl_family *family,
+							const char *group)
 {
 	struct genl_mcast *mcast;
 
-	if (!family)
+	if (unlikely(!family))
 		return false;
 
 	mcast = l_queue_find(family->mcast_list, match_mcast_name,
@@ -1164,15 +1179,17 @@ bool l_genl_family_has_group(struct l_genl_family *family, const char *group)
 	return true;
 }
 
-unsigned int l_genl_family_register(struct l_genl_family *family,
-				const char *group, l_genl_msg_func_t callback,
-				void *user_data, l_genl_destroy_func_t destroy)
+LIB_EXPORT unsigned int l_genl_family_register(struct l_genl_family *family,
+						const char *group,
+						l_genl_msg_func_t callback,
+						void *user_data,
+						l_genl_destroy_func_t destroy)
 {
 	struct l_genl *genl;
 	struct genl_notify *notify;
 	struct genl_mcast *mcast;
 
-	if (!family || !group)
+	if (unlikely(!family) || unlikely(!group))
 		return 0;
 
 	genl = family->genl;
@@ -1213,7 +1230,8 @@ static bool match_notify_id(const void *a, const void *b)
 	return notify->id == id;
 }
 
-bool l_genl_family_unregister(struct l_genl_family *family, unsigned int id)
+LIB_EXPORT bool l_genl_family_unregister(struct l_genl_family *family,
+							unsigned int id)
 {
 	struct l_genl *genl;
 	struct genl_notify *notify;
-- 
2.0.5

[PATCH 2/4] genl: Add a message builder API to help creating complex nl messages

[Tomasz Bursztyka]

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

---
 ell/genl.c | 134 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 ell/genl.h |  18 +++++++++
 2 files changed, 152 insertions(+)

diff --git a/ell/genl.c b/ell/genl.c
index 90c0864..c49c363 100644
--- a/ell/genl.c
+++ b/ell/genl.c
@@ -34,6 +34,7 @@
 #include "io.h"
 #include "netlink-private.h"
 #include "genl.h"
+#include "genl-private.h"
 #include "private.h"
 
 struct l_genl {
@@ -114,6 +115,12 @@ struct l_genl_family {
 	unsigned int nlctrl_cmd;
 };
 
+struct l_genl_msg_builder {
+	struct l_genl_msg *message;
+	struct l_queue *nests;
+	struct nlattr *last_nested;
+};
+
 static void destroy_request(void *data)
 {
 	struct genl_request *request = data;
@@ -642,6 +649,16 @@ LIB_EXPORT struct l_genl_msg *l_genl_msg_new_sized(uint8_t cmd, uint32_t size)
 	return msg_alloc(cmd, 0x00, size);
 }
 
+const void *_genl_msg_get_as_raw_data(struct l_genl_msg *msg, uint32_t *len)
+{
+	if (!msg)
+		return NULL;
+
+	*len = msg->len;
+
+	return msg->data;
+}
+
 LIB_EXPORT struct l_genl_msg *l_genl_msg_ref(struct l_genl_msg *msg)
 {
 	if (unlikely(!msg))
@@ -1252,3 +1269,120 @@ LIB_EXPORT bool l_genl_family_unregister(struct l_genl_family *family,
 
 	return true;
 }
+
+LIB_EXPORT struct l_genl_msg_builder *l_genl_msg_builder_new(
+						struct l_genl_msg *message)
+{
+	struct l_genl_msg_builder *ret;
+
+	if (unlikely(!message))
+		return NULL;
+
+	ret = l_new(struct l_genl_msg_builder, 1);
+	ret->message = l_genl_msg_ref(message);
+
+	return ret;
+}
+
+static inline void builder_message_realign(struct l_genl_msg_builder *builder)
+{
+	if (!builder->last_nested)
+		return;
+
+	if (!builder->nests)
+		builder->message->len = NLA_ALIGN(builder->message->len);
+
+	builder->last_nested = NULL;
+}
+
+LIB_EXPORT bool l_genl_msg_builder_append_attribute(
+					struct l_genl_msg_builder *builder,
+					uint16_t type, uint16_t len,
+					const void *data)
+{
+	if (unlikely(!builder))
+		return false;
+
+	builder_message_realign(builder);
+
+	return l_genl_msg_append_attr(builder->message, type, len, data);
+}
+
+LIB_EXPORT bool l_genl_msg_builder_enter_nested(
+					struct l_genl_msg_builder *builder,
+					uint16_t type)
+{
+	struct nlattr *nla;
+
+	if (unlikely(!builder))
+		return false;
+
+	if (!builder->nests) {
+		builder->nests = l_queue_new();
+		if (!builder->nests)
+			return false;
+	}
+
+	builder_message_realign(builder);
+
+	nla = builder->message->data + builder->message->len;
+	nla->nla_len = NLA_HDRLEN;
+	nla->nla_type = type;
+
+	if (!l_genl_msg_append_attr(builder->message, type, 0, NULL))
+		return false;
+
+	return l_queue_push_head(builder->nests, nla);
+}
+
+LIB_EXPORT bool l_genl_msg_builder_leave_nested(
+					struct l_genl_msg_builder *builder)
+{
+	struct nlattr *nla;
+	uint32_t len;
+
+	if (unlikely(!builder) || unlikely(!builder->nests))
+		return false;
+
+	nla = l_queue_pop_head(builder->nests);
+	if (!nla)
+		return false;
+
+	if (l_queue_isempty(builder->nests)) {
+		l_queue_destroy(builder->nests, NULL);
+		builder->nests = NULL;
+	}
+
+	// In case nothing has been appended into the nested attribute
+	// it could try to be smart and revert the message back and return true
+	len = builder->message->len - ((void *)nla - builder->message->data);
+	if (len <= NLA_HDRLEN)
+		return false;
+
+	nla->nla_len = len;
+
+	builder->last_nested = nla;
+
+	return true;
+}
+
+LIB_EXPORT void l_genl_msg_builder_destroy(struct l_genl_msg_builder *builder)
+{
+	if (unlikely(!builder))
+		return;
+
+	l_queue_destroy(builder->nests, NULL);
+	l_genl_msg_unref(builder->message);
+	l_free(builder);
+}
+
+LIB_EXPORT struct l_genl_msg *l_genl_msg_builder_finalize(
+					struct l_genl_msg_builder *builder)
+{
+	if (unlikely(!builder) || unlikely(builder->nests))
+		return NULL;
+
+	builder_message_realign(builder);
+
+	return builder->message;
+}
diff --git a/ell/genl.h b/ell/genl.h
index c628b8c..a31db1c 100644
--- a/ell/genl.h
+++ b/ell/genl.h
@@ -112,6 +112,24 @@ unsigned int l_genl_family_register(struct l_genl_family *family,
 				void *user_data, l_genl_destroy_func_t destroy);
 bool l_genl_family_unregister(struct l_genl_family *family, unsigned int id);
 
+struct l_genl_msg_builder;
+
+struct l_genl_msg_builder *l_genl_msg_builder_new(struct l_genl_msg *msg);
+
+bool l_genl_msg_builder_append_attribute(struct l_genl_msg_builder *builder,
+						uint16_t type, uint16_t len,
+						const void *data);
+
+bool l_genl_msg_builder_enter_nested(struct l_genl_msg_builder *builder,
+								uint16_t type);
+
+bool l_genl_msg_builder_leave_nested(struct l_genl_msg_builder *builder);
+
+void l_genl_msg_builder_destroy(struct l_genl_msg_builder *builder);
+
+struct l_genl_msg *l_genl_msg_builder_finalize(
+					struct l_genl_msg_builder *builder);
+
 #ifdef __cplusplus
 }
 #endif
-- 
2.0.5

[PATCH 3/4] genl: Add a function to grab the message's raw data for unit purpose

[Tomasz Bursztyka]

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

This is a private function only meant to be used in unit test.
---
 Makefile.am        |  1 +
 ell/genl-private.h | 25 +++++++++++++++++++++++++
 2 files changed, 26 insertions(+)
 create mode 100644 ell/genl-private.h

diff --git a/Makefile.am b/Makefile.am
index cea1b7a..ba3ea4f 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -60,6 +60,7 @@ ell_libell_la_SOURCES = $(linux_headers) \
 			ell/netlink-private.h \
 			ell/netlink.c \
 			ell/genl.c \
+			ell/genl-private.h \
 			ell/dbus-private.h \
 			ell/dbus.c \
 			ell/dbus-kernel.c \
diff --git a/ell/genl-private.h b/ell/genl-private.h
new file mode 100644
index 0000000..d1b769b
--- /dev/null
+++ b/ell/genl-private.h
@@ -0,0 +1,25 @@
+/*
+ *
+ *  Embedded Linux library
+ *
+ *  Copyright (C) 2015  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
+ *
+ */
+
+#include <ell/genl.h>
+
+const void *_genl_msg_get_as_raw_data(struct l_genl_msg *msg, uint32_t *len);
-- 
2.0.5

[PATCH 4/4] unit: Add a message builder unit test for genl

[Tomasz Bursztyka]

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

This tests quickly 2 mesasges, one with a nested attribute and one
without.
---
 unit/test-genl.c | 136 +++++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 117 insertions(+), 19 deletions(-)

diff --git a/unit/test-genl.c b/unit/test-genl.c
index 1764133..b5801f4 100644
--- a/unit/test-genl.c
+++ b/unit/test-genl.c
@@ -24,37 +24,135 @@
 #include <config.h>
 #endif
 
-#include <sys/socket.h>
+#include <assert.h>
+#include <linux/if_ether.h>
+#include <linux/nl80211.h>
 
 #include <ell/ell.h>
+#include <ell/genl-private.h>
 
-static void do_debug(const char *str, void *user_data)
-{
-	const char *prefix = user_data;
+struct key_builder {
+	uint32_t index;
+	uint8_t key_index;
+	uint8_t mac[ETH_ALEN];
+	const char *tk;
+	size_t tk_len;
+	const char *rsc;
+	size_t rsc_len;
+	uint32_t cipher;
+	bool multicast;
+	const char *verification;
+	uint32_t verification_len;
+};
 
-	l_info("%s%s", prefix, str);
-}
+static struct key_builder nl80211_nested_key = {
+	.index = 2,
+	.key_index = 1,
+	.tk = "\x6f\xe8\x7a\x88\x52\x3a\x9f\x8a"
+		"\x10\x2f\x8d\x2d\x99\xd8\xab\xbf",
+	.tk_len = 16,
+	.rsc = "\x0c\x00\x00\x00\x00\x00",
+	.rsc_len = 6,
+	.cipher = 0x000FAC04,
+	.multicast = true,
+	.verification = "\x00\x00\x00\x00\x00\x00\x00\x00"
+			"\x00\x00\x00\x00\x00\x00\x00\x00"
+			"\x00\x00\x00\x00\x08\x00\x03\x00"
+			"\x02\x00\x00\x00\x14\x00\x07\x00"
+			"\x6f\xe8\x7a\x88\x52\x3a\x9f\x8a"
+			"\x10\x2f\x8d\x2d\x99\xd8\xab\xbf"
+			"\x08\x00\x09\x00\x04\xac\x0f\x00"
+			"\x0a\x00\x0a\x00\x0c\x00\x00\x00"
+			"\x00\x00\x00\x00\x08\x00\x6e\x00"
+			"\x04\x00\x02\x00\x05\x00\x08\x00"
+			"\x01\x00\x00\x00",
+	.verification_len = 84,
+};
 
-static void idle_callback(void *user_data)
-{
-	l_main_quit();
-}
+static struct key_builder nl80211_key = {
+	.index = 2,
+	.key_index = 0,
+	.mac = { 0xA0, 0x88, 0xB4, 0x21, 0xE2, 0xC8 },
+	.tk = "\x4a\x8c\x82\x09\x4c\xb8\x84\x55"
+		"\x3b\xdd\x28\xd2\xa1\x82\x17\xe4",
+	.tk_len = 16,
+	.rsc = "\x00\x00\x00\x00\x00\x00",
+	.rsc_len = 6,
+	.cipher = 0x000FAC04,
+	.verification = "\x00\x00\x00\x00\x00\x00\x00\x00"
+			"\x00\x00\x00\x00\x00\x00\x00\x00"
+			"\x00\x00\x00\x00\x08\x00\x03\x00"
+			"\x02\x00\x00\x00\x14\x00\x07\x00"
+			"\x4a\x8c\x82\x09\x4c\xb8\x84\x55"
+			"\x3b\xdd\x28\xd2\xa1\x82\x17\xe4"
+			"\x08\x00\x09\x00\x04\xac\x0f\x00"
+			"\x0a\x00\x0a\x00\x00\x00\x00\x00"
+			"\x00\x00\x00\x00\x0a\x00\x06\x00"
+			"\xa0\x88\xb4\x21\xe2\xc8\x00\x00"
+			"\x05\x00\x08\x00\x00\x00\x00\x00",
+	.verification_len = 88,
+};
 
-int main(int argc, char *argv[])
+static void test_genl_msg_builder(const void *data)
 {
-	struct l_genl *genl;
+	const struct key_builder *key = data;
+	struct l_genl_msg *msg;
+	struct l_genl_msg_builder *builder;
+	const void *raw;
+	uint32_t len;
+	int r;
+
+	msg = l_genl_msg_new_sized(NL80211_CMD_NEW_KEY, 512);
+	assert(msg);
+
+	builder = l_genl_msg_builder_new(msg);
+	assert(builder);
 
-	l_log_set_stderr();
+	/* Build the message */
+	l_genl_msg_builder_append_attribute(builder, NL80211_ATTR_IFINDEX,
+							4, &key->index);
+	l_genl_msg_builder_append_attribute(builder, NL80211_ATTR_KEY_DATA,
+							key->tk_len, key->tk);
+	l_genl_msg_builder_append_attribute(builder, NL80211_ATTR_KEY_CIPHER,
+							4, &key->cipher);
 
-	genl = l_genl_new_default();
+	if (key->rsc && key->rsc_len)
+		l_genl_msg_builder_append_attribute(builder,
+							NL80211_ATTR_KEY_SEQ,
+							key->rsc_len,
+							key->rsc);
+	if (key->multicast) {
+		l_genl_msg_builder_enter_nested(builder,
+					NL80211_ATTR_KEY_DEFAULT_TYPES);
+		l_genl_msg_builder_append_attribute(builder,
+					NL80211_KEY_DEFAULT_TYPE_MULTICAST,
+					0, NULL);
+		l_genl_msg_builder_leave_nested(builder);
+	} else
+		l_genl_msg_builder_append_attribute(builder,
+					NL80211_ATTR_MAC, ETH_ALEN, key->mac);
 
-	l_genl_set_debug(genl, do_debug, "[GENL] ", NULL);
+	l_genl_msg_builder_append_attribute(builder, NL80211_ATTR_KEY_IDX,
+							1, &key->key_index);
 
-	l_idle_oneshot(idle_callback, NULL, NULL);
+	msg = l_genl_msg_builder_finalize(builder);
+	assert(msg);
 
-	l_main_run();
+	raw = _genl_msg_get_as_raw_data(msg, &len);
+	assert((len == key->verification_len));
+
+	r = memcmp(raw, key->verification, key->verification_len);
+	assert(!r);
+
+	l_genl_msg_builder_destroy(builder);
+}
+
+int main(int argc, char *argv[])
+{
+	l_test_init(&argc, &argv);
 
-	l_genl_unref(genl);
+	l_test_add("nested msg", test_genl_msg_builder, &nl80211_nested_key);
+	l_test_add("normal msg", test_genl_msg_builder, &nl80211_key);
 
-	return 0;
+	return l_test_run();
 }
-- 
2.0.5

Re: [PATCH 1/4] genl: Normilize functions exports and parameters check as it should

[Denis Kenzior]

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

Hi Tomasz,

On 03/10/2015 06:14 AM, Tomasz Bursztyka wrote:
> Ell follows a certain way of exporting library functions and checking
> parameter's validity, so let's apply these rules to genl.c
> ---
>   ell/genl.c | 152 ++++++++++++++++++++++++++++++++++---------------------------
>   1 file changed, 85 insertions(+), 67 deletions(-)
>

Applied after fixing up the commit header.

Regards,
-Denis

Re: [PATCH 4/4] unit: Add a message builder unit test for genl

[Marcel Holtmann]

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

Hi Tomasz,

> This tests quickly 2 mesasges, one with a nested attribute and one
> without.

we need also test cases with double and triple nested messages. They are used in nl80211.

Regards

Marcel

Re: [PATCH 2/4] genl: Add a message builder API to help creating complex nl messages

[Marcel Holtmann]

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

Hi Tomasz,

> ---
> ell/genl.c | 134 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> ell/genl.h |  18 +++++++++
> 2 files changed, 152 insertions(+)
> 
> diff --git a/ell/genl.c b/ell/genl.c
> index 90c0864..c49c363 100644
> --- a/ell/genl.c
> +++ b/ell/genl.c
> @@ -34,6 +34,7 @@
> #include "io.h"
> #include "netlink-private.h"
> #include "genl.h"
> +#include "genl-private.h"
> #include "private.h"
> 
> struct l_genl {
> @@ -114,6 +115,12 @@ struct l_genl_family {
> 	unsigned int nlctrl_cmd;
> };
> 
> +struct l_genl_msg_builder {
> +	struct l_genl_msg *message;
> +	struct l_queue *nests;
> +	struct nlattr *last_nested;
> +};
> +
> static void destroy_request(void *data)
> {
> 	struct genl_request *request = data;
> @@ -642,6 +649,16 @@ LIB_EXPORT struct l_genl_msg *l_genl_msg_new_sized(uint8_t cmd, uint32_t size)
> 	return msg_alloc(cmd, 0x00, size);
> }
> 
> +const void *_genl_msg_get_as_raw_data(struct l_genl_msg *msg, uint32_t *len)
> +{
> +	if (!msg)
> +		return NULL;
> +
> +	*len = msg->len;
> +
> +	return msg->data;
> +}
> +
> LIB_EXPORT struct l_genl_msg *l_genl_msg_ref(struct l_genl_msg *msg)
> {
> 	if (unlikely(!msg))
> @@ -1252,3 +1269,120 @@ LIB_EXPORT bool l_genl_family_unregister(struct l_genl_family *family,
> 
> 	return true;
> }
> +
> +LIB_EXPORT struct l_genl_msg_builder *l_genl_msg_builder_new(
> +						struct l_genl_msg *message)
> +{
> +	struct l_genl_msg_builder *ret;
> +
> +	if (unlikely(!message))
> +		return NULL;
> +
> +	ret = l_new(struct l_genl_msg_builder, 1);
> +	ret->message = l_genl_msg_ref(message);
> +
> +	return ret;
> +}
> +
> +static inline void builder_message_realign(struct l_genl_msg_builder *builder)
> +{
> +	if (!builder->last_nested)
> +		return;
> +
> +	if (!builder->nests)
> +		builder->message->len = NLA_ALIGN(builder->message->len);
> +
> +	builder->last_nested = NULL;
> +}
> +
> +LIB_EXPORT bool l_genl_msg_builder_append_attribute(
> +					struct l_genl_msg_builder *builder,
> +					uint16_t type, uint16_t len,
> +					const void *data)
> +{
> +	if (unlikely(!builder))
> +		return false;
> +
> +	builder_message_realign(builder);
> +
> +	return l_genl_msg_append_attr(builder->message, type, len, data);
> +}
> +
> +LIB_EXPORT bool l_genl_msg_builder_enter_nested(
> +					struct l_genl_msg_builder *builder,
> +					uint16_t type)
> +{
> +	struct nlattr *nla;
> +
> +	if (unlikely(!builder))
> +		return false;
> +
> +	if (!builder->nests) {
> +		builder->nests = l_queue_new();
> +		if (!builder->nests)
> +			return false;
> +	}

do we really need a queue here? Can we not just store the latest offset. Also how do you do nested of nested attributes?

> +
> +	builder_message_realign(builder);
> +
> +	nla = builder->message->data + builder->message->len;
> +	nla->nla_len = NLA_HDRLEN;
> +	nla->nla_type = type;
> +
> +	if (!l_genl_msg_append_attr(builder->message, type, 0, NULL))
> +		return false;
> +
> +	return l_queue_push_head(builder->nests, nla);
> +}
> +
> +LIB_EXPORT bool l_genl_msg_builder_leave_nested(
> +					struct l_genl_msg_builder *builder)
> +{
> +	struct nlattr *nla;
> +	uint32_t len;
> +
> +	if (unlikely(!builder) || unlikely(!builder->nests))
> +		return false;
> +
> +	nla = l_queue_pop_head(builder->nests);
> +	if (!nla)
> +		return false;
> +
> +	if (l_queue_isempty(builder->nests)) {
> +		l_queue_destroy(builder->nests, NULL);
> +		builder->nests = NULL;
> +	}
> +
> +	// In case nothing has been appended into the nested attribute
> +	// it could try to be smart and revert the message back and return true

this is not our comment style. Please respect the coding style.

> +	len = builder->message->len - ((void *)nla - builder->message->data);
> +	if (len <= NLA_HDRLEN)
> +		return false;
> +
> +	nla->nla_len = len;
> +
> +	builder->last_nested = nla;
> +
> +	return true;
> +}
> +
> +LIB_EXPORT void l_genl_msg_builder_destroy(struct l_genl_msg_builder *builder)
> +{
> +	if (unlikely(!builder))
> +		return;
> +
> +	l_queue_destroy(builder->nests, NULL);
> +	l_genl_msg_unref(builder->message);
> +	l_free(builder);
> +}
> +
> +LIB_EXPORT struct l_genl_msg *l_genl_msg_builder_finalize(
> +					struct l_genl_msg_builder *builder)
> +{
> +	if (unlikely(!builder) || unlikely(builder->nests))
> +		return NULL;
> +
> +	builder_message_realign(builder);
> +
> +	return builder->message;
> +}
> diff --git a/ell/genl.h b/ell/genl.h
> index c628b8c..a31db1c 100644
> --- a/ell/genl.h
> +++ b/ell/genl.h
> @@ -112,6 +112,24 @@ unsigned int l_genl_family_register(struct l_genl_family *family,
> 				void *user_data, l_genl_destroy_func_t destroy);
> bool l_genl_family_unregister(struct l_genl_family *family, unsigned int id);
> 
> +struct l_genl_msg_builder;
> +
> +struct l_genl_msg_builder *l_genl_msg_builder_new(struct l_genl_msg *msg);
> +
> +bool l_genl_msg_builder_append_attribute(struct l_genl_msg_builder *builder,
> +						uint16_t type, uint16_t len,
> +						const void *data);
> +
> +bool l_genl_msg_builder_enter_nested(struct l_genl_msg_builder *builder,
> +								uint16_t type);
> +
> +bool l_genl_msg_builder_leave_nested(struct l_genl_msg_builder *builder);
> +
> +void l_genl_msg_builder_destroy(struct l_genl_msg_builder *builder);
> +
> +struct l_genl_msg *l_genl_msg_builder_finalize(
> +					struct l_genl_msg_builder *builder);

So I saw that l_dbus_message_builder does a similar thing. We give in a message to start from and then we get that message back out when finalizing it. Has anything changed in between or why are we doing this?

Regards

Marcel

Re: [PATCH 2/4] genl: Add a message builder API to help creating complex nl messages

[Tomasz Bursztyka]

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

Hi Marcel,

>> +LIB_EXPORT bool l_genl_msg_builder_enter_nested(
>> +					struct l_genl_msg_builder *builder,
>> +					uint16_t type)
>> +{
>> +	struct nlattr *nla;
>> +
>> +	if (unlikely(!builder))
>> +		return false;
>> +
>> +	if (!builder->nests) {
>> +		builder->nests = l_queue_new();
>> +		if (!builder->nests)
>> +			return false;
>> +	}
> do we really need a queue here? Can we not just store the latest offset. Also how do you do nested of nested attributes?

Nested of nested exists, in nftables for instance.
Unless we prefer to have a fixed depth of nested of nested (like 3, 4...?),
we need such dynamically allocated list.

I haven't tested this case, but we have to handle it.
Unless we plan to use that code for nl80211 only.

>> +	}
>> +
>> +	// In case nothing has been appended into the nested attribute
>> +	// it could try to be smart and revert the message back and return true
> this is not our comment style. Please respect the coding style.

Actually, it's more like a question that needs an answer. I don't want 
this comment to go in anyway.
Or let's just forget entirely about this.

>> +struct l_genl_msg *l_genl_msg_builder_finalize(
>> +					struct l_genl_msg_builder *builder);
> So I saw that l_dbus_message_builder does a similar thing. We give in a message to start from and then we get that message back out when finalizing it. Has anything changed in between or why are we doing this?

The rule was to follow what DBus API does. But I think it's useless:
- l_genl_msg_builder_new()  it could take a size, and create the msg 
internally. (or have a fixed size of 512bytes, and if really needed 
would realloc relevantly?)
- then if everything went well: l_genl_msg_builder_finalize() would 
return a msg pointer (a reference of it actually), which one we could 
use to send  (and send unref the msg).

Less function to call from client code.
It's actually awkward that you have to create the msg yourself, then 
also a builder with it etc...
when you have also other functions to append attributes directly in the msg.
At least in dbus api you can't append anything BUT through a builder.
(So yes I don't want to follow the DBus Api but finally a bit anyway...)

The naming is too long as well, l_genl_msg_builder_append_attribute() 
mostly.
Suggestions would be welcome. I can strip the "ibute" at the end at least.
And also msg_builder into _msgbld_ or does the lack of vowels makes it 
even crappier?

Tomasz

Re: [PATCH 4/4] unit: Add a message builder unit test for genl

[Marcel Holtmann]

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

Hi Tomasz,

> This tests quickly 2 mesasges, one with a nested attribute and one
> without.
> ---
> unit/test-genl.c | 136 +++++++++++++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 117 insertions(+), 19 deletions(-)
> 
> diff --git a/unit/test-genl.c b/unit/test-genl.c
> index 1764133..b5801f4 100644
> --- a/unit/test-genl.c
> +++ b/unit/test-genl.c
> @@ -24,37 +24,135 @@
> #include <config.h>
> #endif
> 
> -#include <sys/socket.h>
> +#include <assert.h>
> +#include <linux/if_ether.h>
> +#include <linux/nl80211.h>

I rather not include nl80211.h from ELL.

> #include <ell/ell.h>
> +#include <ell/genl-private.h>
> 
> -static void do_debug(const char *str, void *user_data)
> -{
> -	const char *prefix = user_data;
> +struct key_builder {
> +	uint32_t index;
> +	uint8_t key_index;
> +	uint8_t mac[ETH_ALEN];
> +	const char *tk;
> +	size_t tk_len;
> +	const char *rsc;
> +	size_t rsc_len;
> +	uint32_t cipher;
> +	bool multicast;
> +	const char *verification;
> +	uint32_t verification_len;
> +};

I would really prefer if we are building generic tests data structures that can be used here for adding many (read 20+) test cases instead of some copied over from iwd.

Unit tests should be generic and excerise every possible functionality. That way you can ensure that code works and keep ensuring that nobody accidentally broke it.

I mean you have unit/test-dbus-message.c as an example here. It has functions to build the message and a data structure to compare it again. That seems to work pretty well so far. Why not do the same.

> 
> -	l_info("%s%s", prefix, str);
> -}
> +static struct key_builder nl80211_nested_key = {
> +	.index = 2,
> +	.key_index = 1,
> +	.tk = "\x6f\xe8\x7a\x88\x52\x3a\x9f\x8a"
> +		"\x10\x2f\x8d\x2d\x99\xd8\xab\xbf",
> +	.tk_len = 16,
> +	.rsc = "\x0c\x00\x00\x00\x00\x00",
> +	.rsc_len = 6,
> +	.cipher = 0x000FAC04,
> +	.multicast = true,
> +	.verification = "\x00\x00\x00\x00\x00\x00\x00\x00"
> +			"\x00\x00\x00\x00\x00\x00\x00\x00"
> +			"\x00\x00\x00\x00\x08\x00\x03\x00"
> +			"\x02\x00\x00\x00\x14\x00\x07\x00"
> +			"\x6f\xe8\x7a\x88\x52\x3a\x9f\x8a"
> +			"\x10\x2f\x8d\x2d\x99\xd8\xab\xbf"
> +			"\x08\x00\x09\x00\x04\xac\x0f\x00"
> +			"\x0a\x00\x0a\x00\x0c\x00\x00\x00"
> +			"\x00\x00\x00\x00\x08\x00\x6e\x00"
> +			"\x04\x00\x02\x00\x05\x00\x08\x00"
> +			"\x01\x00\x00\x00",
> +	.verification_len = 84,
> +};
> 
> -static void idle_callback(void *user_data)
> -{
> -	l_main_quit();
> -}
> +static struct key_builder nl80211_key = {
> +	.index = 2,
> +	.key_index = 0,
> +	.mac = { 0xA0, 0x88, 0xB4, 0x21, 0xE2, 0xC8 },
> +	.tk = "\x4a\x8c\x82\x09\x4c\xb8\x84\x55"
> +		"\x3b\xdd\x28\xd2\xa1\x82\x17\xe4",
> +	.tk_len = 16,
> +	.rsc = "\x00\x00\x00\x00\x00\x00",
> +	.rsc_len = 6,
> +	.cipher = 0x000FAC04,
> +	.verification = "\x00\x00\x00\x00\x00\x00\x00\x00"
> +			"\x00\x00\x00\x00\x00\x00\x00\x00"
> +			"\x00\x00\x00\x00\x08\x00\x03\x00"
> +			"\x02\x00\x00\x00\x14\x00\x07\x00"
> +			"\x4a\x8c\x82\x09\x4c\xb8\x84\x55"
> +			"\x3b\xdd\x28\xd2\xa1\x82\x17\xe4"
> +			"\x08\x00\x09\x00\x04\xac\x0f\x00"
> +			"\x0a\x00\x0a\x00\x00\x00\x00\x00"
> +			"\x00\x00\x00\x00\x0a\x00\x06\x00"
> +			"\xa0\x88\xb4\x21\xe2\xc8\x00\x00"
> +			"\x05\x00\x08\x00\x00\x00\x00\x00",
> +	.verification_len = 88,
> +};
> 
> -int main(int argc, char *argv[])
> +static void test_genl_msg_builder(const void *data)
> {
> -	struct l_genl *genl;
> +	const struct key_builder *key = data;
> +	struct l_genl_msg *msg;
> +	struct l_genl_msg_builder *builder;
> +	const void *raw;
> +	uint32_t len;
> +	int r;
> +
> +	msg = l_genl_msg_new_sized(NL80211_CMD_NEW_KEY, 512);
> +	assert(msg);
> +
> +	builder = l_genl_msg_builder_new(msg);
> +	assert(builder);
> 
> -	l_log_set_stderr();
> +	/* Build the message */
> +	l_genl_msg_builder_append_attribute(builder, NL80211_ATTR_IFINDEX,
> +							4, &key->index);
> +	l_genl_msg_builder_append_attribute(builder, NL80211_ATTR_KEY_DATA,
> +							key->tk_len, key->tk);
> +	l_genl_msg_builder_append_attribute(builder, NL80211_ATTR_KEY_CIPHER,
> +							4, &key->cipher);
> 
> -	genl = l_genl_new_default();
> +	if (key->rsc && key->rsc_len)
> +		l_genl_msg_builder_append_attribute(builder,
> +							NL80211_ATTR_KEY_SEQ,
> +							key->rsc_len,
> +							key->rsc);
> +	if (key->multicast) {
> +		l_genl_msg_builder_enter_nested(builder,
> +					NL80211_ATTR_KEY_DEFAULT_TYPES);
> +		l_genl_msg_builder_append_attribute(builder,
> +					NL80211_KEY_DEFAULT_TYPE_MULTICAST,
> +					0, NULL);
> +		l_genl_msg_builder_leave_nested(builder);
> +	} else
> +		l_genl_msg_builder_append_attribute(builder,
> +					NL80211_ATTR_MAC, ETH_ALEN, key->mac);
> 
> -	l_genl_set_debug(genl, do_debug, "[GENL] ", NULL);
> +	l_genl_msg_builder_append_attribute(builder, NL80211_ATTR_KEY_IDX,
> +							1, &key->key_index);
> 
> -	l_idle_oneshot(idle_callback, NULL, NULL);
> +	msg = l_genl_msg_builder_finalize(builder);
> +	assert(msg);
> 
> -	l_main_run();
> +	raw = _genl_msg_get_as_raw_data(msg, &len);
> +	assert((len == key->verification_len));
> +
> +	r = memcmp(raw, key->verification, key->verification_len);
> +	assert(!r);
> +
> +	l_genl_msg_builder_destroy(builder);
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +	l_test_init(&argc, &argv);
> 
> -	l_genl_unref(genl);
> +	l_test_add("nested msg", test_genl_msg_builder, &nl80211_nested_key);
> +	l_test_add("normal msg", test_genl_msg_builder, &nl80211_key);
> 
> -	return 0;
> +	return l_test_run();
> }

I really do not understand why you had to rewrite my test tool here and take the existing functionality out. There is no mentioning anywhere that this can no longer be done. And if you would have followed the D-Bus examples in ELL, then this looks more fitting with a new unit test like unit/test-genl-msg.c or similar anyway.

Regards

Marcel

Re: [PATCH 4/4] unit: Add a message builder unit test for genl

[Tomasz Bursztyka]

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

Hi Marcel,

>> -#include <sys/socket.h>
>> +#include <assert.h>
>> +#include <linux/if_ether.h>
>> +#include <linux/nl80211.h>
> I rather not include nl80211.h from ELL.

Ok, so redefining the constants should be the way.

>> #include <ell/ell.h>
>> +#include <ell/genl-private.h>
>>
>> -static void do_debug(const char *str, void *user_data)
>> -{
>> -	const char *prefix = user_data;
>> +struct key_builder {
>> +	uint32_t index;
>> +	uint8_t key_index;
>> +	uint8_t mac[ETH_ALEN];
>> +	const char *tk;
>> +	size_t tk_len;
>> +	const char *rsc;
>> +	size_t rsc_len;
>> +	uint32_t cipher;
>> +	bool multicast;
>> +	const char *verification;
>> +	uint32_t verification_len;
>> +};
> I would really prefer if we are building generic tests data structures that can be used here for adding many (read 20+) test cases instead of some copied over from iwd.
>
> Unit tests should be generic and excerise every possible functionality. That way you can ensure that code works and keep ensuring that nobody accidentally broke it.
>
> I mean you have unit/test-dbus-message.c as an example here. It has functions to build the message and a data structure to compare it again. That seems to work pretty well so far. Why not do the same.

Ok

>> +int main(int argc, char *argv[])
>> +{
>> +	l_test_init(&argc, &argv);
>>
>> -	l_genl_unref(genl);
>> +	l_test_add("nested msg", test_genl_msg_builder, &nl80211_nested_key);
>> +	l_test_add("normal msg", test_genl_msg_builder, &nl80211_key);
>>
>> -	return 0;
>> +	return l_test_run();
>> }
> I really do not understand why you had to rewrite my test tool here and take the existing functionality out. There is no mentioning anywhere that this can no longer be done. And if you would have followed the D-Bus examples in ELL, then this looks more fitting with a new unit test like unit/test-genl-msg.c or similar anyway.

Ok, I'll let test-genl.c alone. Looks like Denis did the test-genl-msg.c 
so I push the builder thing there.

Tomasz

Re: [PATCH 2/4] genl: Add a message builder API to help creating complex nl messages

[Tomasz Bursztyka]

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

Hi Marcel,

>> do we really need a queue here? Can we not just store the latest 
>> offset. Also how do you do nested of nested attributes?
>
> Nested of nested exists, in nftables for instance.
> Unless we prefer to have a fixed depth of nested of nested (like 3, 
> 4...?),
> we need such dynamically allocated list.
>
> I haven't tested this case, but we have to handle it.
> Unless we plan to use that code for nl80211 only.
>
>>> +    }
>>> +
>>> +    // In case nothing has been appended into the nested attribute
>>> +    // it could try to be smart and revert the message back and 
>>> return true
>> this is not our comment style. Please respect the coding style.
>
> Actually, it's more like a question that needs an answer. I don't want 
> this comment to go in anyway.
> Or let's just forget entirely about this.
>
>>> +struct l_genl_msg *l_genl_msg_builder_finalize(
>>> +                    struct l_genl_msg_builder *builder);
>> So I saw that l_dbus_message_builder does a similar thing. We give in 
>> a message to start from and then we get that message back out when 
>> finalizing it. Has anything changed in between or why are we doing this?
>
> The rule was to follow what DBus API does. But I think it's useless:
> - l_genl_msg_builder_new()  it could take a size, and create the msg 
> internally. (or have a fixed size of 512bytes, and if really needed 
> would realloc relevantly?)
> - then if everything went well: l_genl_msg_builder_finalize() would 
> return a msg pointer (a reference of it actually), which one we could 
> use to send  (and send unref the msg).
>
> Less function to call from client code.
> It's actually awkward that you have to create the msg yourself, then 
> also a builder with it etc...
> when you have also other functions to append attributes directly in 
> the msg.
> At least in dbus api you can't append anything BUT through a builder.
> (So yes I don't want to follow the DBus Api but finally a bit anyway...)
>
> The naming is too long as well, l_genl_msg_builder_append_attribute() 
> mostly.
> Suggestions would be welcome. I can strip the "ibute" at the end at 
> least.
> And also msg_builder into _msgbld_ or does the lack of vowels makes it 
> even crappier?


Any comment on this?
Would be nice if I can solve all that, api and unit test, at once.

Tomasz