* libmnl misc patches Nov 11
@ 2010-11-10 23:07 Jan Engelhardt
2010-11-10 23:08 ` [PATCH 1/5] socket: constify a struct sockaddr_nl Jan Engelhardt
` (4 more replies)
0 siblings, 5 replies; 20+ messages in thread
From: Jan Engelhardt @ 2010-11-10 23:07 UTC (permalink / raw)
To: pablo; +Cc: netfilter-devel
Please review, and if ok, apply.
__
The following changes since commit 415a51b34293eac33a7d2152b909f89e5af4ed10:
Merge branch 'master' of git://dev.medozas.de/libmnl (2010-11-07 20:53:15 +0100)
are available in the git repository at:
git://dev.medozas.de/libmnl master
Jan Engelhardt (5):
socket: constify a struct sockaddr_nl
include: use C++ headers in C++ mode
nlmsg: use bool for all _ok functions
attr: remove redundant check for NULL
attr: avoid multiple definition of hidden variable
include/libmnl/libmnl.h | 41 +++++++++++++++++++++--------------------
src/attr.c | 23 ++++++++---------------
src/nlmsg.c | 2 +-
src/socket.c | 2 +-
4 files changed, 31 insertions(+), 37 deletions(-)
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 1/5] socket: constify a struct sockaddr_nl
2010-11-10 23:07 libmnl misc patches Nov 11 Jan Engelhardt
@ 2010-11-10 23:08 ` Jan Engelhardt
2010-11-11 12:56 ` Pablo Neira Ayuso
2010-11-10 23:08 ` [PATCH 2/5] include: use C++ headers in C++ mode Jan Engelhardt
` (3 subsequent siblings)
4 siblings, 1 reply; 20+ messages in thread
From: Jan Engelhardt @ 2010-11-10 23:08 UTC (permalink / raw)
To: pablo; +Cc: netfilter-devel
Signed-off-by: Jan Engelhardt <jengelh@medozas.de>
---
src/socket.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/socket.c b/src/socket.c
index c9afd34..cc997df 100644
--- a/src/socket.c
+++ b/src/socket.c
@@ -171,7 +171,7 @@ int mnl_socket_bind(struct mnl_socket *nl, unsigned int groups, pid_t pid)
*/
int mnl_socket_sendto(const struct mnl_socket *nl, const void *buf, size_t len)
{
- struct sockaddr_nl snl = {
+ static const struct sockaddr_nl snl = {
.nl_family = AF_NETLINK
};
return sendto(nl->fd, buf, len, 0,
--
1.7.1
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 2/5] include: use C++ headers in C++ mode
2010-11-10 23:07 libmnl misc patches Nov 11 Jan Engelhardt
2010-11-10 23:08 ` [PATCH 1/5] socket: constify a struct sockaddr_nl Jan Engelhardt
@ 2010-11-10 23:08 ` Jan Engelhardt
2010-11-11 12:57 ` Pablo Neira Ayuso
2013-06-07 8:35 ` Thomas Jarosch
2010-11-10 23:08 ` [PATCH 3/5] nlmsg: use bool for all _ok functions Jan Engelhardt
` (2 subsequent siblings)
4 siblings, 2 replies; 20+ messages in thread
From: Jan Engelhardt @ 2010-11-10 23:08 UTC (permalink / raw)
To: pablo; +Cc: netfilter-devel
Signed-off-by: Jan Engelhardt <jengelh@medozas.de>
---
include/libmnl/libmnl.h | 13 ++++++++-----
1 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/include/libmnl/libmnl.h b/include/libmnl/libmnl.h
index 7094af2..37c502a 100644
--- a/include/libmnl/libmnl.h
+++ b/include/libmnl/libmnl.h
@@ -1,13 +1,16 @@
#ifndef _LIBMNL_H_
#define _LIBMNL_H_
-#include <stdio.h>
-#include <stdint.h>
+#ifdef __cplusplus
+# include <cstdio>
+# include <cstdint>
+#else
+# include <stdbool.h> /* not in C++ */
+# include <stdio.h>
+# include <stdint.h>
+#endif
#include <sys/socket.h> /* for sa_family_t */
#include <linux/netlink.h>
-#ifndef __cplusplus
-# include <stdbool.h>
-#endif
#ifdef __cplusplus
extern "C" {
--
1.7.1
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 3/5] nlmsg: use bool for all _ok functions
2010-11-10 23:07 libmnl misc patches Nov 11 Jan Engelhardt
2010-11-10 23:08 ` [PATCH 1/5] socket: constify a struct sockaddr_nl Jan Engelhardt
2010-11-10 23:08 ` [PATCH 2/5] include: use C++ headers in C++ mode Jan Engelhardt
@ 2010-11-10 23:08 ` Jan Engelhardt
2010-11-11 12:59 ` Pablo Neira Ayuso
2010-11-10 23:08 ` [PATCH 4/5] attr: remove redundant check for NULL Jan Engelhardt
2010-11-10 23:08 ` [PATCH 5/5] attr: avoid multiple definition of hidden variable Jan Engelhardt
4 siblings, 1 reply; 20+ messages in thread
From: Jan Engelhardt @ 2010-11-10 23:08 UTC (permalink / raw)
To: pablo; +Cc: netfilter-devel
Signed-off-by: Jan Engelhardt <jengelh@medozas.de>
---
include/libmnl/libmnl.h | 4 ++--
src/attr.c | 2 +-
src/nlmsg.c | 2 +-
3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/include/libmnl/libmnl.h b/include/libmnl/libmnl.h
index 37c502a..43388e5 100644
--- a/include/libmnl/libmnl.h
+++ b/include/libmnl/libmnl.h
@@ -52,7 +52,7 @@ extern struct nlmsghdr *mnl_nlmsg_put_header(void *buf);
extern void *mnl_nlmsg_put_extra_header(struct nlmsghdr *nlh, size_t size);
/* Netlink message iterators */
-extern int mnl_nlmsg_ok(const struct nlmsghdr *nlh, int len);
+extern bool mnl_nlmsg_ok(const struct nlmsghdr *nlh, int len);
extern struct nlmsghdr *mnl_nlmsg_next(const struct nlmsghdr *nlh, int *len);
/* Netlink sequence tracking */
@@ -121,7 +121,7 @@ extern int mnl_attr_validate(const struct nlattr *attr, enum mnl_attr_data_type
extern int mnl_attr_validate2(const struct nlattr *attr, enum mnl_attr_data_type type, size_t len);
/* TLV iterators */
-extern int mnl_attr_ok(const struct nlattr *attr, int len);
+extern bool mnl_attr_ok(const struct nlattr *attr, int len);
extern struct nlattr *mnl_attr_next(const struct nlattr *attr, int *len);
#define mnl_attr_for_each(attr, nlh, offset) \
diff --git a/src/attr.c b/src/attr.c
index c60e1f4..69fda0b 100644
--- a/src/attr.c
+++ b/src/attr.c
@@ -90,7 +90,7 @@ void *mnl_attr_get_payload(const struct nlattr *attr)
* The len parameter may be negative in the case of malformed messages during
* attribute iteration, that is why we use a signed integer.
*/
-int mnl_attr_ok(const struct nlattr *attr, int len)
+bool mnl_attr_ok(const struct nlattr *attr, int len)
{
return len >= (int)sizeof(struct nlattr) &&
attr->nla_len >= sizeof(struct nlattr) &&
diff --git a/src/nlmsg.c b/src/nlmsg.c
index f89a9ec..b5c5c29 100644
--- a/src/nlmsg.c
+++ b/src/nlmsg.c
@@ -156,7 +156,7 @@ void *mnl_nlmsg_get_payload_offset(const struct nlmsghdr *nlh, size_t offset)
* The len parameter may become negative in malformed messages during message
* iteration, that is why we use a signed integer.
*/
-int mnl_nlmsg_ok(const struct nlmsghdr *nlh, int len)
+bool mnl_nlmsg_ok(const struct nlmsghdr *nlh, int len)
{
return len >= (int)sizeof(struct nlmsghdr) &&
nlh->nlmsg_len >= sizeof(struct nlmsghdr) &&
--
1.7.1
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 4/5] attr: remove redundant check for NULL
2010-11-10 23:07 libmnl misc patches Nov 11 Jan Engelhardt
` (2 preceding siblings ...)
2010-11-10 23:08 ` [PATCH 3/5] nlmsg: use bool for all _ok functions Jan Engelhardt
@ 2010-11-10 23:08 ` Jan Engelhardt
2010-11-11 13:02 ` Pablo Neira Ayuso
2010-11-10 23:08 ` [PATCH 5/5] attr: avoid multiple definition of hidden variable Jan Engelhardt
4 siblings, 1 reply; 20+ messages in thread
From: Jan Engelhardt @ 2010-11-10 23:08 UTC (permalink / raw)
To: pablo; +Cc: netfilter-devel
Calling mnl_attr_parse with cb==NULL is pointless, because the
function will do nothing else.
Signed-off-by: Jan Engelhardt <jengelh@medozas.de>
---
src/attr.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/attr.c b/src/attr.c
index 69fda0b..e22a8ac 100644
--- a/src/attr.c
+++ b/src/attr.c
@@ -260,7 +260,7 @@ int mnl_attr_parse(const struct nlmsghdr *nlh, unsigned int offset,
int len = nlh->nlmsg_len - MNL_NLMSG_HDRLEN - MNL_ALIGN(offset);
while (mnl_attr_ok(attr, len)) {
- if (cb && (ret = cb(attr, data)) <= MNL_CB_STOP)
+ if ((ret = cb(attr, data)) <= MNL_CB_STOP)
return ret;
attr = mnl_attr_next(attr, &len);
}
@@ -289,7 +289,7 @@ int mnl_attr_parse_nested(const struct nlattr *nested,
int len = mnl_attr_get_payload_len(nested);
while (mnl_attr_ok(attr, len)) {
- if (cb && (ret = cb(attr, data)) <= MNL_CB_STOP)
+ if ((ret = cb(attr, data)) <= MNL_CB_STOP)
return ret;
attr = mnl_attr_next(attr, &len);
}
--
1.7.1
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 5/5] attr: avoid multiple definition of hidden variable
2010-11-10 23:07 libmnl misc patches Nov 11 Jan Engelhardt
` (3 preceding siblings ...)
2010-11-10 23:08 ` [PATCH 4/5] attr: remove redundant check for NULL Jan Engelhardt
@ 2010-11-10 23:08 ` Jan Engelhardt
2010-11-11 13:08 ` Pablo Neira Ayuso
4 siblings, 1 reply; 20+ messages in thread
From: Jan Engelhardt @ 2010-11-10 23:08 UTC (permalink / raw)
To: pablo; +Cc: netfilter-devel
When nesting two mnl_attr_for_each loops, the __len__ variable will be
declared twice, eliciting a warning when -Wshadow is turned on. There
can also be warnings in pre-C99 because declarations and code are
mixed. Do without any temporaries that are not explicitly specified as
macro parameters.
Signed-off-by: Jan Engelhardt <jengelh@medozas.de>
---
include/libmnl/libmnl.h | 24 +++++++++++-------------
src/attr.c | 17 +++++------------
2 files changed, 16 insertions(+), 25 deletions(-)
diff --git a/include/libmnl/libmnl.h b/include/libmnl/libmnl.h
index 43388e5..9520a07 100644
--- a/include/libmnl/libmnl.h
+++ b/include/libmnl/libmnl.h
@@ -122,19 +122,17 @@ extern int mnl_attr_validate2(const struct nlattr *attr, enum mnl_attr_data_type
/* TLV iterators */
extern bool mnl_attr_ok(const struct nlattr *attr, int len);
-extern struct nlattr *mnl_attr_next(const struct nlattr *attr, int *len);
-
-#define mnl_attr_for_each(attr, nlh, offset) \
- int __len__ = mnl_nlmsg_get_payload_len(nlh); \
- for (attr = mnl_nlmsg_get_payload_offset(nlh, offset); \
- mnl_attr_ok(attr, __len__); \
- attr = mnl_attr_next(attr, &(__len__)))
-
-#define mnl_attr_for_each_nested(attr, nest) \
- int __len__ = mnl_attr_get_len(nest); \
- for (attr = mnl_attr_get_payload(nest); \
- mnl_attr_ok(attr, __len__); \
- attr = mnl_attr_next(attr, &(__len__)))
+extern struct nlattr *mnl_attr_next(const struct nlattr *attr);
+
+#define mnl_attr_for_each(attr, nlh, offset) \
+ for ((attr) = mnl_nlmsg_get_payload_offset((nlh), (offset)); \
+ mnl_attr_ok((attr), mnl_nlmsg_get_payload_tail(nlh) - (void *)(attr)); \
+ (attr) = mnl_attr_next(attr))
+
+#define mnl_attr_for_each_nested(attr, nest) \
+ for ((attr) = mnl_attr_get_payload(nest); \
+ mnl_attr_ok((attr), mnl_attr_get_payload(attr) + mnl_attr_get_payload_len(attr) - (void *)(attr)); \
+ (attr) = mnl_attr_next(attr))
/* TLV callback-based attribute parsers */
typedef int (*mnl_attr_cb_t)(const struct nlattr *attr, void *data);
diff --git a/src/attr.c b/src/attr.c
index e22a8ac..5137395 100644
--- a/src/attr.c
+++ b/src/attr.c
@@ -106,9 +106,8 @@ bool mnl_attr_ok(const struct nlattr *attr, int len)
* as parameter. You have to use mnl_attr_ok() to ensure that the next
* attribute is valid.
*/
-struct nlattr *mnl_attr_next(const struct nlattr *attr, int *len)
+struct nlattr *mnl_attr_next(const struct nlattr *attr)
{
- *len -= MNL_ALIGN(attr->nla_len);
return (struct nlattr *)((void *)attr + MNL_ALIGN(attr->nla_len));
}
@@ -256,14 +255,11 @@ int mnl_attr_parse(const struct nlmsghdr *nlh, unsigned int offset,
mnl_attr_cb_t cb, void *data)
{
int ret = MNL_CB_OK;
- const struct nlattr *attr = mnl_nlmsg_get_payload_offset(nlh, offset);
- int len = nlh->nlmsg_len - MNL_NLMSG_HDRLEN - MNL_ALIGN(offset);
+ const struct nlattr *attr;
- while (mnl_attr_ok(attr, len)) {
+ mnl_attr_for_each(attr, nlh, offset)
if ((ret = cb(attr, data)) <= MNL_CB_STOP)
return ret;
- attr = mnl_attr_next(attr, &len);
- }
return ret;
}
@@ -285,14 +281,11 @@ int mnl_attr_parse_nested(const struct nlattr *nested,
mnl_attr_cb_t cb, void *data)
{
int ret = MNL_CB_OK;
- const struct nlattr *attr = mnl_attr_get_payload(nested);
- int len = mnl_attr_get_payload_len(nested);
+ const struct nlattr *attr;
- while (mnl_attr_ok(attr, len)) {
+ mnl_attr_for_each_nested(attr, nested)
if ((ret = cb(attr, data)) <= MNL_CB_STOP)
return ret;
- attr = mnl_attr_next(attr, &len);
- }
return ret;
}
--
1.7.1
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 1/5] socket: constify a struct sockaddr_nl
2010-11-10 23:08 ` [PATCH 1/5] socket: constify a struct sockaddr_nl Jan Engelhardt
@ 2010-11-11 12:56 ` Pablo Neira Ayuso
0 siblings, 0 replies; 20+ messages in thread
From: Pablo Neira Ayuso @ 2010-11-11 12:56 UTC (permalink / raw)
To: Jan Engelhardt; +Cc: netfilter-devel
On 11/11/10 00:08, Jan Engelhardt wrote:
> Signed-off-by: Jan Engelhardt <jengelh@medozas.de>
Applied, thanks.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/5] include: use C++ headers in C++ mode
2010-11-10 23:08 ` [PATCH 2/5] include: use C++ headers in C++ mode Jan Engelhardt
@ 2010-11-11 12:57 ` Pablo Neira Ayuso
2010-11-11 12:59 ` Jan Engelhardt
2013-06-07 8:35 ` Thomas Jarosch
1 sibling, 1 reply; 20+ messages in thread
From: Pablo Neira Ayuso @ 2010-11-11 12:57 UTC (permalink / raw)
To: Jan Engelhardt; +Cc: netfilter-devel
On 11/11/10 00:08, Jan Engelhardt wrote:
> diff --git a/include/libmnl/libmnl.h b/include/libmnl/libmnl.h
> index 7094af2..37c502a 100644
> --- a/include/libmnl/libmnl.h
> +++ b/include/libmnl/libmnl.h
> @@ -1,13 +1,16 @@
> #ifndef _LIBMNL_H_
> #define _LIBMNL_H_
>
> -#include <stdio.h>
> -#include <stdint.h>
> +#ifdef __cplusplus
> +# include <cstdio>
> +# include <cstdint>
> +#else
> +# include <stdbool.h> /* not in C++ */
> +# include <stdio.h>
> +# include <stdint.h>
> +#endif
> #include <sys/socket.h> /* for sa_family_t */
> #include <linux/netlink.h>
> -#ifndef __cplusplus
> -# include <stdbool.h>
> -#endif
A c++ example compiles fine without this here, so do we really need
these extra ifdef's?
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/5] nlmsg: use bool for all _ok functions
2010-11-10 23:08 ` [PATCH 3/5] nlmsg: use bool for all _ok functions Jan Engelhardt
@ 2010-11-11 12:59 ` Pablo Neira Ayuso
0 siblings, 0 replies; 20+ messages in thread
From: Pablo Neira Ayuso @ 2010-11-11 12:59 UTC (permalink / raw)
To: Jan Engelhardt; +Cc: netfilter-devel
On 11/11/10 00:08, Jan Engelhardt wrote:
> Signed-off-by: Jan Engelhardt <jengelh@medozas.de>
Applied, thanks.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/5] include: use C++ headers in C++ mode
2010-11-11 12:57 ` Pablo Neira Ayuso
@ 2010-11-11 12:59 ` Jan Engelhardt
2010-11-11 13:15 ` Pablo Neira Ayuso
0 siblings, 1 reply; 20+ messages in thread
From: Jan Engelhardt @ 2010-11-11 12:59 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: netfilter-devel
On Thursday 2010-11-11 13:57, Pablo Neira Ayuso wrote:
>On 11/11/10 00:08, Jan Engelhardt wrote:
>> diff --git a/include/libmnl/libmnl.h b/include/libmnl/libmnl.h
>> index 7094af2..37c502a 100644
>> --- a/include/libmnl/libmnl.h
>> +++ b/include/libmnl/libmnl.h
>> @@ -1,13 +1,16 @@
>> #ifndef _LIBMNL_H_
>> #define _LIBMNL_H_
>>
>> -#include <stdio.h>
>> -#include <stdint.h>
>> +#ifdef __cplusplus
>> +# include <cstdio>
>> +# include <cstdint>
>> +#else
>> +# include <stdbool.h> /* not in C++ */
>> +# include <stdio.h>
>> +# include <stdint.h>
>> +#endif
>> #include <sys/socket.h> /* for sa_family_t */
>> #include <linux/netlink.h>
>> -#ifndef __cplusplus
>> -# include <stdbool.h>
>> -#endif
>
>A c++ example compiles fine without this here, so do we really need
>these extra ifdef's?
Well there is a purpose for having <cXXX> over <XXX.h> in C++. IIRC it
was proper namespacing of Standard Library functions.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 4/5] attr: remove redundant check for NULL
2010-11-10 23:08 ` [PATCH 4/5] attr: remove redundant check for NULL Jan Engelhardt
@ 2010-11-11 13:02 ` Pablo Neira Ayuso
0 siblings, 0 replies; 20+ messages in thread
From: Pablo Neira Ayuso @ 2010-11-11 13:02 UTC (permalink / raw)
To: Jan Engelhardt; +Cc: netfilter-devel
On 11/11/10 00:08, Jan Engelhardt wrote:
> Calling mnl_attr_parse with cb==NULL is pointless, because the
> function will do nothing else.
Indeed, applied.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 5/5] attr: avoid multiple definition of hidden variable
2010-11-10 23:08 ` [PATCH 5/5] attr: avoid multiple definition of hidden variable Jan Engelhardt
@ 2010-11-11 13:08 ` Pablo Neira Ayuso
2010-11-11 13:25 ` Jan Engelhardt
0 siblings, 1 reply; 20+ messages in thread
From: Pablo Neira Ayuso @ 2010-11-11 13:08 UTC (permalink / raw)
To: Jan Engelhardt; +Cc: netfilter-devel
On 11/11/10 00:08, Jan Engelhardt wrote:
> When nesting two mnl_attr_for_each loops, the __len__ variable will be
> declared twice, eliciting a warning when -Wshadow is turned on. There
> can also be warnings in pre-C99 because declarations and code are
> mixed. Do without any temporaries that are not explicitly specified as
> macro parameters.
I like this spot, some question below:
> Signed-off-by: Jan Engelhardt <jengelh@medozas.de>
[...]
> diff --git a/src/attr.c b/src/attr.c
> index e22a8ac..5137395 100644
> --- a/src/attr.c
> +++ b/src/attr.c
> @@ -106,9 +106,8 @@ bool mnl_attr_ok(const struct nlattr *attr, int len)
> * as parameter. You have to use mnl_attr_ok() to ensure that the next
> * attribute is valid.
> */
> -struct nlattr *mnl_attr_next(const struct nlattr *attr, int *len)
> +struct nlattr *mnl_attr_next(const struct nlattr *attr)
> {
> - *len -= MNL_ALIGN(attr->nla_len);
> return (struct nlattr *)((void *)attr + MNL_ALIGN(attr->nla_len));
> }
If we remove the len parameter from mnl_attr_next(), we may access
memory that may be out of the message boundary in mnl_attr_ok().
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/5] include: use C++ headers in C++ mode
2010-11-11 12:59 ` Jan Engelhardt
@ 2010-11-11 13:15 ` Pablo Neira Ayuso
2010-11-13 18:19 ` Jan Engelhardt
0 siblings, 1 reply; 20+ messages in thread
From: Pablo Neira Ayuso @ 2010-11-11 13:15 UTC (permalink / raw)
To: Jan Engelhardt; +Cc: netfilter-devel
On 11/11/10 13:59, Jan Engelhardt wrote:
> On Thursday 2010-11-11 13:57, Pablo Neira Ayuso wrote:
>
>> On 11/11/10 00:08, Jan Engelhardt wrote:
>>> diff --git a/include/libmnl/libmnl.h b/include/libmnl/libmnl.h
>>> index 7094af2..37c502a 100644
>>> --- a/include/libmnl/libmnl.h
>>> +++ b/include/libmnl/libmnl.h
>>> @@ -1,13 +1,16 @@
>>> #ifndef _LIBMNL_H_
>>> #define _LIBMNL_H_
>>>
>>> -#include <stdio.h>
>>> -#include <stdint.h>
>>> +#ifdef __cplusplus
>>> +# include <cstdio>
>>> +# include <cstdint>
>>> +#else
>>> +# include <stdbool.h> /* not in C++ */
>>> +# include <stdio.h>
>>> +# include <stdint.h>
>>> +#endif
>>> #include <sys/socket.h> /* for sa_family_t */
>>> #include <linux/netlink.h>
>>> -#ifndef __cplusplus
>>> -# include <stdbool.h>
>>> -#endif
>>
>> A c++ example compiles fine without this here, so do we really need
>> these extra ifdef's?
>
> Well there is a purpose for having <cXXX> over <XXX.h> in C++. IIRC it
> was proper namespacing of Standard Library functions.
I'm looking at other library code but I don't find any doing this. If
you can point to any, I'd appreciate it.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 5/5] attr: avoid multiple definition of hidden variable
2010-11-11 13:08 ` Pablo Neira Ayuso
@ 2010-11-11 13:25 ` Jan Engelhardt
2010-11-11 17:47 ` Pablo Neira Ayuso
0 siblings, 1 reply; 20+ messages in thread
From: Jan Engelhardt @ 2010-11-11 13:25 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: netfilter-devel
On Thursday 2010-11-11 14:08, Pablo Neira Ayuso wrote:
>On 11/11/10 00:08, Jan Engelhardt wrote:
>> When nesting two mnl_attr_for_each loops, the __len__ variable will be
>> declared twice, eliciting a warning when -Wshadow is turned on. There
>> can also be warnings in pre-C99 because declarations and code are
>> mixed. Do without any temporaries that are not explicitly specified as
>> macro parameters.
>
>I like this spot, some question below:
>
>> -struct nlattr *mnl_attr_next(const struct nlattr *attr, int *len)
>> +struct nlattr *mnl_attr_next(const struct nlattr *attr)
>> {
>> - *len -= MNL_ALIGN(attr->nla_len);
>> return (struct nlattr *)((void *)attr + MNL_ALIGN(attr->nla_len));
>> }
>
>If we remove the len parameter from mnl_attr_next(), we may access
>memory that may be out of the message boundary in mnl_attr_ok().
Not that I can see; mnl_attr_ok tests for len >= sizeof(struct nlattr),
and len is tail minus attr.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 5/5] attr: avoid multiple definition of hidden variable
2010-11-11 13:25 ` Jan Engelhardt
@ 2010-11-11 17:47 ` Pablo Neira Ayuso
2010-11-11 20:53 ` Jan Engelhardt
0 siblings, 1 reply; 20+ messages in thread
From: Pablo Neira Ayuso @ 2010-11-11 17:47 UTC (permalink / raw)
To: Jan Engelhardt; +Cc: netfilter-devel
On 11/11/10 14:25, Jan Engelhardt wrote:
>
> On Thursday 2010-11-11 14:08, Pablo Neira Ayuso wrote:
>> On 11/11/10 00:08, Jan Engelhardt wrote:
>>> When nesting two mnl_attr_for_each loops, the __len__ variable will be
>>> declared twice, eliciting a warning when -Wshadow is turned on. There
>>> can also be warnings in pre-C99 because declarations and code are
>>> mixed. Do without any temporaries that are not explicitly specified as
>>> macro parameters.
>>
>> I like this spot, some question below:
>>
>>> -struct nlattr *mnl_attr_next(const struct nlattr *attr, int *len)
>>> +struct nlattr *mnl_attr_next(const struct nlattr *attr)
>>> {
>>> - *len -= MNL_ALIGN(attr->nla_len);
>>> return (struct nlattr *)((void *)attr + MNL_ALIGN(attr->nla_len));
>>> }
>>
>> If we remove the len parameter from mnl_attr_next(), we may access
>> memory that may be out of the message boundary in mnl_attr_ok().
>
> Not that I can see; mnl_attr_ok tests for len >= sizeof(struct nlattr),
> and len is tail minus attr.
mnl_attr_for_each() in your patch is OK, sorry. But, here:
+#define mnl_attr_for_each_nested(attr, nest) \
+ for ((attr) = mnl_attr_get_payload(nest); \
+ mnl_attr_ok((attr), mnl_attr_get_payload(attr) +
mnl_attr_get_payload_len(attr) - (void *)(attr)); \
+ (attr) = mnl_attr_next(attr))
Once we iterate over the last attribute in the nest, we iterate again to
check if there's any next. Then, mnl_attr_get_payload may access
attr->len for an attribute that doesn't belong the nest or (if the nest
is in the end of the message) an out of bound message access.
I think that we can add mnl_attr_get_payload_tail to make tail minus
attr, like in mnl_attr_for_each().
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 5/5] attr: avoid multiple definition of hidden variable
2010-11-11 17:47 ` Pablo Neira Ayuso
@ 2010-11-11 20:53 ` Jan Engelhardt
0 siblings, 0 replies; 20+ messages in thread
From: Jan Engelhardt @ 2010-11-11 20:53 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: netfilter-devel
On Thursday 2010-11-11 18:47, Pablo Neira Ayuso wrote:
>
>mnl_attr_for_each() in your patch is OK, sorry. But, here:
>
>+#define mnl_attr_for_each_nested(attr, nest) \
>+ for ((attr) = mnl_attr_get_payload(nest); \
>+ mnl_attr_ok((attr), mnl_attr_get_payload(attr) +
>mnl_attr_get_payload_len(attr) - (void *)(attr)); \
>+ (attr) = mnl_attr_next(attr))
>
>Once we iterate over the last attribute in the nest, we iterate again to
>check if there's any next. Then, mnl_attr_get_payload may access
>attr->len for an attribute that doesn't belong the nest or (if the nest
>is in the end of the message) an out of bound message access.
Indeed. Should have been
mnl_attr_ok((attr), mnl_attr_get_payload(nest) +
mnl_attr_get_payload_len(nest) - (void *)(attr))
>I think that we can add mnl_attr_get_payload_tail to make tail minus
>attr, like in mnl_attr_for_each().
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/5] include: use C++ headers in C++ mode
2010-11-11 13:15 ` Pablo Neira Ayuso
@ 2010-11-13 18:19 ` Jan Engelhardt
2010-11-16 10:04 ` Pablo Neira Ayuso
0 siblings, 1 reply; 20+ messages in thread
From: Jan Engelhardt @ 2010-11-13 18:19 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: netfilter-devel
On Thursday 2010-11-11 14:15, Pablo Neira Ayuso wrote:
>>>> -#include <stdio.h>
>>>> -#include <stdint.h>
>>>> +#ifdef __cplusplus
>>>> +# include <cstdio>
>>>> +# include <cstdint>
>>>> +#else
>>>> +# include <stdbool.h> /* not in C++ */
>>>> +# include <stdio.h>
>>>> +# include <stdint.h>
>>>> +#endif
>>>> #include <sys/socket.h> /* for sa_family_t */
>>>> #include <linux/netlink.h>
>>>> -#ifndef __cplusplus
>>>> -# include <stdbool.h>
>>>> -#endif
>>>
>>> A c++ example compiles fine without this here, so do we really need
>>> these extra ifdef's?
>>
>> Well there is a purpose for having <cXXX> over <XXX.h> in C++. IIRC it
>> was proper namespacing of Standard Library functions.
>
>I'm looking at other library code but I don't find any doing this. If
>you can point to any, I'd appreciate it.
/usr/include/gmp.h from libgmp-devel, for example.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/5] include: use C++ headers in C++ mode
2010-11-13 18:19 ` Jan Engelhardt
@ 2010-11-16 10:04 ` Pablo Neira Ayuso
0 siblings, 0 replies; 20+ messages in thread
From: Pablo Neira Ayuso @ 2010-11-16 10:04 UTC (permalink / raw)
To: Jan Engelhardt; +Cc: netfilter-devel
On 13/11/10 19:19, Jan Engelhardt wrote:
> On Thursday 2010-11-11 14:15, Pablo Neira Ayuso wrote:
>>>>> -#include <stdio.h>
>>>>> -#include <stdint.h>
>>>>> +#ifdef __cplusplus
>>>>> +# include <cstdio>
>>>>> +# include <cstdint>
>>>>> +#else
>>>>> +# include <stdbool.h> /* not in C++ */
>>>>> +# include <stdio.h>
>>>>> +# include <stdint.h>
>>>>> +#endif
>>>>> #include <sys/socket.h> /* for sa_family_t */
>>>>> #include <linux/netlink.h>
>>>>> -#ifndef __cplusplus
>>>>> -# include <stdbool.h>
>>>>> -#endif
>>>>
>>>> A c++ example compiles fine without this here, so do we really need
>>>> these extra ifdef's?
>>>
>>> Well there is a purpose for having <cXXX> over <XXX.h> in C++. IIRC it
>>> was proper namespacing of Standard Library functions.
>>
>> I'm looking at other library code but I don't find any doing this. If
>> you can point to any, I'd appreciate it.
>
> /usr/include/gmp.h from libgmp-devel, for example.
Applied thanks Jan.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/5] include: use C++ headers in C++ mode
2010-11-10 23:08 ` [PATCH 2/5] include: use C++ headers in C++ mode Jan Engelhardt
2010-11-11 12:57 ` Pablo Neira Ayuso
@ 2013-06-07 8:35 ` Thomas Jarosch
2013-06-08 13:38 ` Jan Engelhardt
1 sibling, 1 reply; 20+ messages in thread
From: Thomas Jarosch @ 2013-06-07 8:35 UTC (permalink / raw)
To: netfilter-devel; +Cc: Jan Engelhardt
Hi Jan,
On Thursday, 11. November 2010 00:08:01 you wrote:
> Signed-off-by: Jan Engelhardt <jengelh@medozas.de>
> ---
> include/libmnl/libmnl.h | 13 ++++++++-----
> 1 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/include/libmnl/libmnl.h b/include/libmnl/libmnl.h
> index 7094af2..37c502a 100644
> --- a/include/libmnl/libmnl.h
> +++ b/include/libmnl/libmnl.h
> @@ -1,13 +1,16 @@
> #ifndef _LIBMNL_H_
> #define _LIBMNL_H_
>
> -#include <stdio.h>
> -#include <stdint.h>
> +#ifdef __cplusplus
> +# include <cstdio>
> +# include <cstdint>
> +#else
> +# include <stdbool.h> /* not in C++ */
> +# include <stdio.h>
> +# include <stdint.h>
> +#endif
just a side note regarding this old commit:
The header "<cstdint>" is part of the C++11 standard.
(http://www.cplusplus.com/reference/cstdint/)
g++ forces you to enable C++11 mode if you want to use it.
Tested with g++ (GCC) 4.4.4.
Do we really need this? It compiles fine without
the special headers inside the __cplusplus part.
Pablo asked the same back in 2010 ;)
Best regards,
Thomas
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/5] include: use C++ headers in C++ mode
2013-06-07 8:35 ` Thomas Jarosch
@ 2013-06-08 13:38 ` Jan Engelhardt
0 siblings, 0 replies; 20+ messages in thread
From: Jan Engelhardt @ 2013-06-08 13:38 UTC (permalink / raw)
To: Thomas Jarosch; +Cc: netfilter-devel
On Friday 2013-06-07 10:35, Thomas Jarosch wrote:
>> -#include <stdio.h>
>> -#include <stdint.h>
>> +#ifdef __cplusplus
>> +# include <cstdio>
>> +# include <cstdint>
>> +#else
>> +# include <stdbool.h> /* not in C++ */
>> +# include <stdio.h>
>> +# include <stdint.h>
>> +#endif
>
>The header "<cstdint>" is part of the C++11 standard.
>(http://www.cplusplus.com/reference/cstdint/)
>
>g++ forces you to enable C++11 mode if you want to use it.
>Tested with g++ (GCC) 4.4.4.
>
>Do we really need this? It compiles fine without
>the special headers inside the __cplusplus part.
>Pablo asked the same back in 2010 ;)
Add an extra condition:
#if defined(__cplusplus) && __cplusplus >= 201100L
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2013-06-08 13:38 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-11-10 23:07 libmnl misc patches Nov 11 Jan Engelhardt
2010-11-10 23:08 ` [PATCH 1/5] socket: constify a struct sockaddr_nl Jan Engelhardt
2010-11-11 12:56 ` Pablo Neira Ayuso
2010-11-10 23:08 ` [PATCH 2/5] include: use C++ headers in C++ mode Jan Engelhardt
2010-11-11 12:57 ` Pablo Neira Ayuso
2010-11-11 12:59 ` Jan Engelhardt
2010-11-11 13:15 ` Pablo Neira Ayuso
2010-11-13 18:19 ` Jan Engelhardt
2010-11-16 10:04 ` Pablo Neira Ayuso
2013-06-07 8:35 ` Thomas Jarosch
2013-06-08 13:38 ` Jan Engelhardt
2010-11-10 23:08 ` [PATCH 3/5] nlmsg: use bool for all _ok functions Jan Engelhardt
2010-11-11 12:59 ` Pablo Neira Ayuso
2010-11-10 23:08 ` [PATCH 4/5] attr: remove redundant check for NULL Jan Engelhardt
2010-11-11 13:02 ` Pablo Neira Ayuso
2010-11-10 23:08 ` [PATCH 5/5] attr: avoid multiple definition of hidden variable Jan Engelhardt
2010-11-11 13:08 ` Pablo Neira Ayuso
2010-11-11 13:25 ` Jan Engelhardt
2010-11-11 17:47 ` Pablo Neira Ayuso
2010-11-11 20:53 ` Jan Engelhardt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).