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

[Marcel Holtmann <marcel@holtmann.org>]

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