[PATCH 0/2] Improve genl with a message builder

[PATCH 0/2] Improve genl with a message builder

[Tomasz Bursztyka]

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

Hi,

I could not test yet the whole patch set so it's sent here for a quick review.

First patch is there since I realized genl lacked of those LIB_EXPORT and unlikely().
Minor stuff so.

Second patch is the interesting one.

I am currently building a unit test for it. Getting some reliable nested based messages
to compare with is the annoyingly time consuming part here.

Tomasz Bursztyka (2):
  genl: Normilize functions exports and parameters check as it should
  genl: Add a message builder API to help creating complex nl messages

 ell/genl.c | 275 ++++++++++++++++++++++++++++++++++++++++++++++---------------
 ell/genl.h |  18 ++++
 2 files changed, 226 insertions(+), 67 deletions(-)

-- 
2.0.5

[PATCH 1/2] 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/2] genl: Add a message builder API to help creating complex nl messages

[Tomasz Bursztyka]

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

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

diff --git a/ell/genl.c b/ell/genl.c
index 90c0864..b006b3b 100644
--- a/ell/genl.c
+++ b/ell/genl.c
@@ -114,6 +114,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;
@@ -1252,3 +1258,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 - nla->nla_len;
+	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

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

[Tomasz Bursztyka]

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

s/Normilize/Normalize ...

Re: [PATCH 0/2] Improve genl with a message builder

[Marcel Holtmann]

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

Hi Tomasz,

> I could not test yet the whole patch set so it's sent here for a quick review.
> 
> First patch is there since I realized genl lacked of those LIB_EXPORT and unlikely().
> Minor stuff so.
> 
> Second patch is the interesting one.
> 
> I am currently building a unit test for it. Getting some reliable nested based messages
> to compare with is the annoyingly time consuming part here.

I do not know what will be so "annoyingly time consuming" in getting nested generic netlink messages for your unit tests. We have iwmon and it can create PCAP files. So start iwmon -w <file> and run wpa_supplicant to connect to your access point. Open Wireshark and you have your raw data that you can copy and put into a unit test. It is really as simple as that.

Unit tests for low level protocols are essential. The more the better. I would not accept patches that do not have unit tests attached with it. Spending extra time on unit tests is time well spent. It will save you a lot headaches later on.

Regards

Marcel

Re: [PATCH 0/2] Improve genl with a message builder

[Tomasz Bursztyka]

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

Hi Marcel,

My sentence was not clear, sorry. I was not criticizing the unit test 
process, I really do want it also,
instead of blindly using the api.

It's just finding out proper raw messages to test against. I found one 
command that has
one nested attribute, but I also would like to get some more complicated 
ones as this api will
be used in every places in our code. It's more time consuming and less 
fun than writing the
api itself ;)  (and yes I am using iwmon hopefully)

Tomasz