[PATCH] genl: Add l_genl_msg_attr_append_data

[PATCH] genl: Add l_genl_msg_attr_append_data

[Andrew Zaborowski]

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

Try to minimise the amount of buffers allocated and copied by clients
when building a message attribute that is a concatenation of other
buffers, such as an IE sequence.  The client would the following three
calls:

l_genl_msg_enter_nested
l_genl_msg_attr_append_data
l_genl_msg_leave_nested
---
 ell/genl.c | 26 ++++++++++++++++++++++++++
 ell/genl.h |  1 +
 2 files changed, 27 insertions(+)

diff --git a/ell/genl.c b/ell/genl.c
index 85e447a..aefea5f 100644
--- a/ell/genl.c
+++ b/ell/genl.c
@@ -828,14 +828,40 @@ LIB_EXPORT bool l_genl_msg_leave_nested(struct l_genl_msg *msg)
 	if (unlikely(msg->nesting_level == 0))
 		return false;
 
+	if (NLA_ALIGN(msg->len) > msg->size)
+		return false;
+
 	nla = msg->nests[msg->nesting_level - 1];
 	nla->nla_len = msg->len - nla->nla_len;
 
 	msg->nesting_level -= 1;
 
+	msg->len = NLA_ALIGN(msg->len);
+
 	return true;
 }
 
+LIB_EXPORT void *l_genl_msg_attr_append_data(struct l_genl_msg *msg,
+						uint16_t len)
+{
+	void *ptr;
+
+	if (unlikely(!msg))
+		return false;
+
+	if (msg->len + len > msg->size)
+		return false;
+
+	if (unlikely(msg->nesting_level == 0))
+		return false;
+
+	ptr = msg->data + msg->len;
+
+	msg->len += len;
+
+	return ptr;
+}
+
 #define NLA_OK(nla,len)         ((len) >= (int) sizeof(struct nlattr) && \
 				(nla)->nla_len >= sizeof(struct nlattr) && \
 				(nla)->nla_len <= (len))
diff --git a/ell/genl.h b/ell/genl.h
index 5b37d55..5bfa459 100644
--- a/ell/genl.h
+++ b/ell/genl.h
@@ -73,6 +73,7 @@ bool l_genl_msg_append_attr(struct l_genl_msg *msg, uint16_t type,
 					uint16_t len, const void *data);
 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);
+void *l_genl_msg_attr_append_data(struct l_genl_msg *msg, uint16_t len);
 
 bool l_genl_attr_init(struct l_genl_attr *attr, struct l_genl_msg *msg);
 bool l_genl_attr_next(struct l_genl_attr *attr, uint16_t *type,
-- 
2.7.4

Re: [PATCH] genl: Add l_genl_msg_attr_append_data

[Denis Kenzior]

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

Hi Andrew,

On 10/05/2016 05:45 PM, Andrew Zaborowski wrote:
> Try to minimise the amount of buffers allocated and copied by clients
> when building a message attribute that is a concatenation of other
> buffers, such as an IE sequence.  The client would the following three
> calls:
>
> l_genl_msg_enter_nested
> l_genl_msg_attr_append_data
> l_genl_msg_leave_nested

So the intent here is that instead of:
msg = l_genl_msg_new_sized(...);
buf = alloca();
// memcpy / scribble in buffer
l_genl_msg_append_attr(msg, NL80211_ATTR_IE, buf_len, buf);

we do something like:
msg = l_genl_msg_new_sized(...);
l_genl_msg_enter_nested(msg, NL80211_ATTR_IE);
buf = l_genl_msg_attr_append_data(msg, len);
// scribble in buf
buf = l_genl_msg_attr_append_data(msg, len);
// scribble in buf
l_genl_msg_leave_nested(msg);

?

If so, you're (very creatively) really abusing the intent of 
enter_nested which is used for true nested attributes.  Don't think I 
really like that...

Is l_genl_msg_append_attrv(msg, iov, iov_len) not sufficient?

Alternatively, we should do something like:

l_genl_msg_attr_open(msg, NL80211_ATTR_IE);
l_genl_msg_attr_reserve(msg, len);
l_genl_msg_attr_append(msg, len, data);
l_genl_msg_attr_reserve(msg, len);
l_genl_msg_attr_close(msg);

Regards,
-Denis

Re: [PATCH] genl: Add l_genl_msg_attr_append_data

[Andrzej Zaborowski]

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

Hi Denis,

On 6 October 2016 at 01:46, Denis Kenzior <denkenz@gmail.com> wrote:
> Hi Andrew,
>
> On 10/05/2016 05:45 PM, Andrew Zaborowski wrote:
>>
>> Try to minimise the amount of buffers allocated and copied by clients
>> when building a message attribute that is a concatenation of other
>> buffers, such as an IE sequence.  The client would the following three
>> calls:
>>
>> l_genl_msg_enter_nested
>> l_genl_msg_attr_append_data
>> l_genl_msg_leave_nested
>
>
> So the intent here is that instead of:
> msg = l_genl_msg_new_sized(...);
> buf = alloca();
> // memcpy / scribble in buffer
> l_genl_msg_append_attr(msg, NL80211_ATTR_IE, buf_len, buf);
>
> we do something like:
> msg = l_genl_msg_new_sized(...);
> l_genl_msg_enter_nested(msg, NL80211_ATTR_IE);
> buf = l_genl_msg_attr_append_data(msg, len);
> // scribble in buf
> buf = l_genl_msg_attr_append_data(msg, len);
> // scribble in buf
> l_genl_msg_leave_nested(msg);
>
> ?
>
> If so, you're (very creatively) really abusing the intent of enter_nested
> which is used for true nested attributes.  Don't think I really like that...

Yes, I sent this more as an RFC with the intent to provide a better
named function instread of l_genl_msg_enter_nested but thought that
was a detail.

>
> Is l_genl_msg_append_attrv(msg, iov, iov_len) not sufficient?

Ok, let's do that instead.  I thought there could be additional
chances to avoid memcpys if we return a pointer to write to.

>
> Alternatively, we should do something like:
>
> l_genl_msg_attr_open(msg, NL80211_ATTR_IE);
> l_genl_msg_attr_reserve(msg, len);
> l_genl_msg_attr_append(msg, len, data);
> l_genl_msg_attr_reserve(msg, len);
> l_genl_msg_attr_close(msg);
>
> Regards,
> -Denis
> _______________________________________________
> ell mailing list
> ell(a)lists.01.org
> https://lists.01.org/mailman/listinfo/ell

Best regards