All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/4] can: gw: Netlink related fixes
@ 2012-07-05 12:19 Thomas Graf
  2012-07-05 12:19 ` [PATCH 1/4] can: Don't bump nlmsg_len manually Thomas Graf
                   ` (5 more replies)
  0 siblings, 6 replies; 13+ messages in thread
From: Thomas Graf @ 2012-07-05 12:19 UTC (permalink / raw)
  To: linux-can

Hi

These patches fix some Netlink quirks in the CAN gateway code.
Unfortunately I do not own any CAN hardware so this is compile
tested only. I'm fairly confident though as the changes are
mostly straight forward. I would appreciate if someone could
run them through a test suite or something though.


Thomas Graf (4):
  can: Don't bump nlmsg_len manually
  can: Use nla_policy to validate netlink attributes
  can: Properly fill the netlink header when responding to RTM_GETROUTE
  can: Remove pointless casts

 net/can/gw.c |   90 +++++++++++++++++++++------------------------------------
 1 files changed, 33 insertions(+), 57 deletions(-)

-- 
1.7.7.6


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/4] can: Don't bump nlmsg_len manually
  2012-07-05 12:19 [PATCH 0/4] can: gw: Netlink related fixes Thomas Graf
@ 2012-07-05 12:19 ` Thomas Graf
  2012-07-05 12:19 ` [PATCH 2/4] can: Use nla_policy to validate netlink attributes Thomas Graf
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 13+ messages in thread
From: Thomas Graf @ 2012-07-05 12:19 UTC (permalink / raw)
  To: linux-can

nlmsg_end() will take care of this when we finalize the message.

Signed-off-by: Thomas Graf <tgraf@suug.ch>
---
 net/can/gw.c |   27 +--------------------------
 1 files changed, 1 insertions(+), 26 deletions(-)

diff --git a/net/can/gw.c b/net/can/gw.c
index b41acf2..a3ff980 100644
--- a/net/can/gw.c
+++ b/net/can/gw.c
@@ -462,15 +462,11 @@ static int cgw_put_job(struct sk_buff *skb, struct cgw_job *gwj)
 	if (gwj->handled_frames) {
 		if (nla_put_u32(skb, CGW_HANDLED, gwj->handled_frames) < 0)
 			goto cancel;
-		else
-			nlh->nlmsg_len += NLA_HDRLEN + NLA_ALIGN(sizeof(u32));
 	}
 
 	if (gwj->dropped_frames) {
 		if (nla_put_u32(skb, CGW_DROPPED, gwj->dropped_frames) < 0)
 			goto cancel;
-		else
-			nlh->nlmsg_len += NLA_HDRLEN + NLA_ALIGN(sizeof(u32));
 	}
 
 	/* check non default settings of attributes */
@@ -480,8 +476,6 @@ static int cgw_put_job(struct sk_buff *skb, struct cgw_job *gwj)
 		mb.modtype = gwj->mod.modtype.and;
 		if (nla_put(skb, CGW_MOD_AND, sizeof(mb), &mb) < 0)
 			goto cancel;
-		else
-			nlh->nlmsg_len += NLA_HDRLEN + NLA_ALIGN(sizeof(mb));
 	}
 
 	if (gwj->mod.modtype.or) {
@@ -489,8 +483,6 @@ static int cgw_put_job(struct sk_buff *skb, struct cgw_job *gwj)
 		mb.modtype = gwj->mod.modtype.or;
 		if (nla_put(skb, CGW_MOD_OR, sizeof(mb), &mb) < 0)
 			goto cancel;
-		else
-			nlh->nlmsg_len += NLA_HDRLEN + NLA_ALIGN(sizeof(mb));
 	}
 
 	if (gwj->mod.modtype.xor) {
@@ -498,8 +490,6 @@ static int cgw_put_job(struct sk_buff *skb, struct cgw_job *gwj)
 		mb.modtype = gwj->mod.modtype.xor;
 		if (nla_put(skb, CGW_MOD_XOR, sizeof(mb), &mb) < 0)
 			goto cancel;
-		else
-			nlh->nlmsg_len += NLA_HDRLEN + NLA_ALIGN(sizeof(mb));
 	}
 
 	if (gwj->mod.modtype.set) {
@@ -507,26 +497,18 @@ static int cgw_put_job(struct sk_buff *skb, struct cgw_job *gwj)
 		mb.modtype = gwj->mod.modtype.set;
 		if (nla_put(skb, CGW_MOD_SET, sizeof(mb), &mb) < 0)
 			goto cancel;
-		else
-			nlh->nlmsg_len += NLA_HDRLEN + NLA_ALIGN(sizeof(mb));
 	}
 
 	if (gwj->mod.csumfunc.crc8) {
 		if (nla_put(skb, CGW_CS_CRC8, CGW_CS_CRC8_LEN,
 			    &gwj->mod.csum.crc8) < 0)
 			goto cancel;
-		else
-			nlh->nlmsg_len += NLA_HDRLEN + \
-				NLA_ALIGN(CGW_CS_CRC8_LEN);
 	}
 
 	if (gwj->mod.csumfunc.xor) {
 		if (nla_put(skb, CGW_CS_XOR, CGW_CS_XOR_LEN,
 			    &gwj->mod.csum.xor) < 0)
 			goto cancel;
-		else
-			nlh->nlmsg_len += NLA_HDRLEN + \
-				NLA_ALIGN(CGW_CS_XOR_LEN);
 	}
 
 	if (gwj->gwtype == CGW_TYPE_CAN_CAN) {
@@ -535,23 +517,16 @@ static int cgw_put_job(struct sk_buff *skb, struct cgw_job *gwj)
 			if (nla_put(skb, CGW_FILTER, sizeof(struct can_filter),
 				    &gwj->ccgw.filter) < 0)
 				goto cancel;
-			else
-				nlh->nlmsg_len += NLA_HDRLEN +
-					NLA_ALIGN(sizeof(struct can_filter));
 		}
 
 		if (nla_put_u32(skb, CGW_SRC_IF, gwj->ccgw.src_idx) < 0)
 			goto cancel;
-		else
-			nlh->nlmsg_len += NLA_HDRLEN + NLA_ALIGN(sizeof(u32));
 
 		if (nla_put_u32(skb, CGW_DST_IF, gwj->ccgw.dst_idx) < 0)
 			goto cancel;
-		else
-			nlh->nlmsg_len += NLA_HDRLEN + NLA_ALIGN(sizeof(u32));
 	}
 
-	return skb->len;
+	return nlmsg_end(skb, nlh);
 
 cancel:
 	nlmsg_cancel(skb, nlh);
-- 
1.7.7.6


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/4] can: Use nla_policy to validate netlink attributes
  2012-07-05 12:19 [PATCH 0/4] can: gw: Netlink related fixes Thomas Graf
  2012-07-05 12:19 ` [PATCH 1/4] can: Don't bump nlmsg_len manually Thomas Graf
@ 2012-07-05 12:19 ` Thomas Graf
  2012-07-05 12:19 ` [PATCH 3/4] can: Properly fill the netlink header when responding to RTM_GETROUTE Thomas Graf
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 13+ messages in thread
From: Thomas Graf @ 2012-07-05 12:19 UTC (permalink / raw)
  To: linux-can

Also use nla_get_u32() instead of nla_memcpy() to access u32 attribtues.

Signed-off-by: Thomas Graf <tgraf@suug.ch>
---
 net/can/gw.c |   47 +++++++++++++++++++++++------------------------
 1 files changed, 23 insertions(+), 24 deletions(-)

diff --git a/net/can/gw.c b/net/can/gw.c
index a3ff980..a1c639c 100644
--- a/net/can/gw.c
+++ b/net/can/gw.c
@@ -558,6 +558,18 @@ cont:
 	return skb->len;
 }
 
+static const struct nla_policy cgw_policy[CGW_MAX+1] = {
+	[CGW_MOD_AND]	= { .len = sizeof(struct cgw_frame_mod) },
+	[CGW_MOD_OR]	= { .len = sizeof(struct cgw_frame_mod) },
+	[CGW_MOD_XOR]	= { .len = sizeof(struct cgw_frame_mod) },
+	[CGW_MOD_SET]	= { .len = sizeof(struct cgw_frame_mod) },
+	[CGW_CS_XOR]	= { .len = sizeof(struct cgw_csum_xor) },
+	[CGW_CS_CRC8]	= { .len = sizeof(struct cgw_csum_crc8) },
+	[CGW_SRC_IF]	= { .type = NLA_U32 },
+	[CGW_DST_IF]	= { .type = NLA_U32 },
+	[CGW_FILTER]	= { .len = sizeof(struct can_filter) },
+};
+
 /* check for common and gwtype specific attributes */
 static int cgw_parse_attr(struct nlmsghdr *nlh, struct cf_mod *mod,
 			  u8 gwtype, void *gwtypeattr)
@@ -570,14 +582,14 @@ static int cgw_parse_attr(struct nlmsghdr *nlh, struct cf_mod *mod,
 	/* initialize modification & checksum data space */
 	memset(mod, 0, sizeof(*mod));
 
-	err = nlmsg_parse(nlh, sizeof(struct rtcanmsg), tb, CGW_MAX, NULL);
+	err = nlmsg_parse(nlh, sizeof(struct rtcanmsg), tb, CGW_MAX,
+			  cgw_policy);
 	if (err < 0)
 		return err;
 
 	/* check for AND/OR/XOR/SET modifications */
 
-	if (tb[CGW_MOD_AND] &&
-	    nla_len(tb[CGW_MOD_AND]) == CGW_MODATTR_LEN) {
+	if (tb[CGW_MOD_AND]) {
 		nla_memcpy(&mb, tb[CGW_MOD_AND], CGW_MODATTR_LEN);
 
 		canframecpy(&mod->modframe.and, &mb.cf);
@@ -593,8 +605,7 @@ static int cgw_parse_attr(struct nlmsghdr *nlh, struct cf_mod *mod,
 			mod->modfunc[modidx++] = mod_and_data;
 	}
 
-	if (tb[CGW_MOD_OR] &&
-	    nla_len(tb[CGW_MOD_OR]) == CGW_MODATTR_LEN) {
+	if (tb[CGW_MOD_OR]) {
 		nla_memcpy(&mb, tb[CGW_MOD_OR], CGW_MODATTR_LEN);
 
 		canframecpy(&mod->modframe.or, &mb.cf);
@@ -610,8 +621,7 @@ static int cgw_parse_attr(struct nlmsghdr *nlh, struct cf_mod *mod,
 			mod->modfunc[modidx++] = mod_or_data;
 	}
 
-	if (tb[CGW_MOD_XOR] &&
-	    nla_len(tb[CGW_MOD_XOR]) == CGW_MODATTR_LEN) {
+	if (tb[CGW_MOD_XOR]) {
 		nla_memcpy(&mb, tb[CGW_MOD_XOR], CGW_MODATTR_LEN);
 
 		canframecpy(&mod->modframe.xor, &mb.cf);
@@ -627,8 +637,7 @@ static int cgw_parse_attr(struct nlmsghdr *nlh, struct cf_mod *mod,
 			mod->modfunc[modidx++] = mod_xor_data;
 	}
 
-	if (tb[CGW_MOD_SET] &&
-	    nla_len(tb[CGW_MOD_SET]) == CGW_MODATTR_LEN) {
+	if (tb[CGW_MOD_SET]) {
 		nla_memcpy(&mb, tb[CGW_MOD_SET], CGW_MODATTR_LEN);
 
 		canframecpy(&mod->modframe.set, &mb.cf);
@@ -647,9 +656,7 @@ static int cgw_parse_attr(struct nlmsghdr *nlh, struct cf_mod *mod,
 	/* check for checksum operations after CAN frame modifications */
 	if (modidx) {
 
-		if (tb[CGW_CS_CRC8] &&
-		    nla_len(tb[CGW_CS_CRC8]) == CGW_CS_CRC8_LEN) {
-
+		if (tb[CGW_CS_CRC8]) {
 			struct cgw_csum_crc8 *c = (struct cgw_csum_crc8 *)\
 				nla_data(tb[CGW_CS_CRC8]);
 
@@ -674,9 +681,7 @@ static int cgw_parse_attr(struct nlmsghdr *nlh, struct cf_mod *mod,
 				mod->csumfunc.crc8 = cgw_csum_crc8_neg;
 		}
 
-		if (tb[CGW_CS_XOR] &&
-		    nla_len(tb[CGW_CS_XOR]) == CGW_CS_XOR_LEN) {
-
+		if (tb[CGW_CS_XOR]) {
 			struct cgw_csum_xor *c = (struct cgw_csum_xor *)\
 				nla_data(tb[CGW_CS_XOR]);
 
@@ -710,8 +715,7 @@ static int cgw_parse_attr(struct nlmsghdr *nlh, struct cf_mod *mod,
 		memset(ccgw, 0, sizeof(*ccgw));
 
 		/* check for can_filter in attributes */
-		if (tb[CGW_FILTER] &&
-		    nla_len(tb[CGW_FILTER]) == sizeof(struct can_filter))
+		if (tb[CGW_FILTER])
 			nla_memcpy(&ccgw->filter, tb[CGW_FILTER],
 				   sizeof(struct can_filter));
 
@@ -721,13 +725,8 @@ static int cgw_parse_attr(struct nlmsghdr *nlh, struct cf_mod *mod,
 		if (!tb[CGW_SRC_IF] || !tb[CGW_DST_IF])
 			return err;
 
-		if (nla_len(tb[CGW_SRC_IF]) == sizeof(u32))
-			nla_memcpy(&ccgw->src_idx, tb[CGW_SRC_IF],
-				   sizeof(u32));
-
-		if (nla_len(tb[CGW_DST_IF]) == sizeof(u32))
-			nla_memcpy(&ccgw->dst_idx, tb[CGW_DST_IF],
-				   sizeof(u32));
+		ccgw->src_idx = nla_get_u32(tb[CGW_SRC_IF]);
+		ccgw->dst_idx = nla_get_u32(tb[CGW_DST_IF]);
 
 		/* both indices set to 0 for flushing all routing entries */
 		if (!ccgw->src_idx && !ccgw->dst_idx)
-- 
1.7.7.6


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 3/4] can: Properly fill the netlink header when responding to RTM_GETROUTE
  2012-07-05 12:19 [PATCH 0/4] can: gw: Netlink related fixes Thomas Graf
  2012-07-05 12:19 ` [PATCH 1/4] can: Don't bump nlmsg_len manually Thomas Graf
  2012-07-05 12:19 ` [PATCH 2/4] can: Use nla_policy to validate netlink attributes Thomas Graf
@ 2012-07-05 12:19 ` Thomas Graf
  2012-07-06 20:39   ` Oliver Hartkopp
  2012-07-07  9:18   ` Oliver Hartkopp
  2012-07-05 12:19 ` [PATCH 4/4] can: Remove pointless casts Thomas Graf
                   ` (2 subsequent siblings)
  5 siblings, 2 replies; 13+ messages in thread
From: Thomas Graf @ 2012-07-05 12:19 UTC (permalink / raw)
  To: linux-can

 - set message type to RTM_NEWROUTE
 - relate to original request by inheriting the sequence and port number.
 - set NLM_F_MULTI because it's a dump and more messages will follow

Signed-off-by: Thomas Graf <tgraf@suug.ch>
---
 net/can/gw.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/net/can/gw.c b/net/can/gw.c
index a1c639c..20c36e1 100644
--- a/net/can/gw.c
+++ b/net/can/gw.c
@@ -444,11 +444,14 @@ static int cgw_notifier(struct notifier_block *nb,
 	return NOTIFY_DONE;
 }
 
-static int cgw_put_job(struct sk_buff *skb, struct cgw_job *gwj)
+static int cgw_put_job(struct sk_buff *skb, struct cgw_job *gwj, int type,
+		       u32 pid, u32 seq, int flags)
 {
 	struct cgw_frame_mod mb;
 	struct rtcanmsg *rtcan;
-	struct nlmsghdr *nlh = nlmsg_put(skb, 0, 0, 0, sizeof(*rtcan), 0);
+	struct nlmsghdr *nlh;
+
+	nlh = nlmsg_put(skb, pid, seq, type, sizeof(*rtcan), flags);
 	if (!nlh)
 		return -EMSGSIZE;
 
@@ -546,7 +549,8 @@ static int cgw_dump_jobs(struct sk_buff *skb, struct netlink_callback *cb)
 		if (idx < s_idx)
 			goto cont;
 
-		if (cgw_put_job(skb, gwj) < 0)
+		if (cgw_put_job(skb, gwj, RTM_NEWROUTE, NETLINK_CB(cb->skb).pid,
+		    cb->nlh->nlmsg_seq, NLM_F_MULTI) < 0)
 			break;
 cont:
 		idx++;
-- 
1.7.7.6


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 4/4] can: Remove pointless casts
  2012-07-05 12:19 [PATCH 0/4] can: gw: Netlink related fixes Thomas Graf
                   ` (2 preceding siblings ...)
  2012-07-05 12:19 ` [PATCH 3/4] can: Properly fill the netlink header when responding to RTM_GETROUTE Thomas Graf
@ 2012-07-05 12:19 ` Thomas Graf
  2012-07-05 16:16 ` [PATCH 0/4] can: gw: Netlink related fixes Oliver Hartkopp
  2012-07-10 20:55 ` Marc Kleine-Budde
  5 siblings, 0 replies; 13+ messages in thread
From: Thomas Graf @ 2012-07-05 12:19 UTC (permalink / raw)
  To: linux-can

No need to cast return value of nla_data()

Signed-off-by: Thomas Graf <tgraf@suug.ch>
---
 net/can/gw.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/can/gw.c b/net/can/gw.c
index 20c36e1..b54d5e6 100644
--- a/net/can/gw.c
+++ b/net/can/gw.c
@@ -661,8 +661,7 @@ static int cgw_parse_attr(struct nlmsghdr *nlh, struct cf_mod *mod,
 	if (modidx) {
 
 		if (tb[CGW_CS_CRC8]) {
-			struct cgw_csum_crc8 *c = (struct cgw_csum_crc8 *)\
-				nla_data(tb[CGW_CS_CRC8]);
+			struct cgw_csum_crc8 *c = nla_data(tb[CGW_CS_CRC8]);
 
 			err = cgw_chk_csum_parms(c->from_idx, c->to_idx,
 						 c->result_idx);
@@ -686,8 +685,7 @@ static int cgw_parse_attr(struct nlmsghdr *nlh, struct cf_mod *mod,
 		}
 
 		if (tb[CGW_CS_XOR]) {
-			struct cgw_csum_xor *c = (struct cgw_csum_xor *)\
-				nla_data(tb[CGW_CS_XOR]);
+			struct cgw_csum_xor *c = nla_data(tb[CGW_CS_XOR]);
 
 			err = cgw_chk_csum_parms(c->from_idx, c->to_idx,
 						 c->result_idx);
-- 
1.7.7.6


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/4] can: gw: Netlink related fixes
  2012-07-05 12:19 [PATCH 0/4] can: gw: Netlink related fixes Thomas Graf
                   ` (3 preceding siblings ...)
  2012-07-05 12:19 ` [PATCH 4/4] can: Remove pointless casts Thomas Graf
@ 2012-07-05 16:16 ` Oliver Hartkopp
  2012-07-10 20:55 ` Marc Kleine-Budde
  5 siblings, 0 replies; 13+ messages in thread
From: Oliver Hartkopp @ 2012-07-05 16:16 UTC (permalink / raw)
  To: Thomas Graf; +Cc: linux-can

On 05.07.2012 14:19, Thomas Graf wrote:


> These patches fix some Netlink quirks in the CAN gateway code.
> Unfortunately I do not own any CAN hardware so this is compile
> tested only. I'm fairly confident though as the changes are
> mostly straight forward. I would appreciate if someone could
> run them through a test suite or something though.
> 
> 
> Thomas Graf (4):
>   can: Don't bump nlmsg_len manually
>   can: Use nla_policy to validate netlink attributes
>   can: Properly fill the netlink header when responding to RTM_GETROUTE
>   can: Remove pointless casts
> 
>  net/can/gw.c |   90 +++++++++++++++++++++------------------------------------
>  1 files changed, 33 insertions(+), 57 deletions(-)
> 


Hi Thomas,

thanks for the cleanups!

The can-gw was my first netlink based project and i did the best i knew.

From the first sight it looks good. I now also got behind the nla_policy
trick, which checks the minimal length too.

I'll test the patches this weekend with the virtual CAN interfaces (no CAN HW
needed), so that we probably catch the current development window.

Best regards,
Oliver

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/4] can: Properly fill the netlink header when responding to RTM_GETROUTE
  2012-07-05 12:19 ` [PATCH 3/4] can: Properly fill the netlink header when responding to RTM_GETROUTE Thomas Graf
@ 2012-07-06 20:39   ` Oliver Hartkopp
  2012-07-09  9:43     ` Thomas Graf
  2012-07-07  9:18   ` Oliver Hartkopp
  1 sibling, 1 reply; 13+ messages in thread
From: Oliver Hartkopp @ 2012-07-06 20:39 UTC (permalink / raw)
  To: Thomas Graf; +Cc: linux-can

On 05.07.2012 14:19, Thomas Graf wrote:

>  - set message type to RTM_NEWROUTE
>  - relate to original request by inheriting the sequence and port number.
>  - set NLM_F_MULTI because it's a dump and more messages will follow
> 
> Signed-off-by: Thomas Graf <tgraf@suug.ch>
> ---
>  net/can/gw.c |   10 +++++++---
>  1 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/net/can/gw.c b/net/can/gw.c
> index a1c639c..20c36e1 100644
> --- a/net/can/gw.c
> +++ b/net/can/gw.c
> @@ -444,11 +444,14 @@ static int cgw_notifier(struct notifier_block *nb,
>  	return NOTIFY_DONE;
>  }
>  
> -static int cgw_put_job(struct sk_buff *skb, struct cgw_job *gwj)
> +static int cgw_put_job(struct sk_buff *skb, struct cgw_job *gwj, int type,
> +		       u32 pid, u32 seq, int flags)
>  {
>  	struct cgw_frame_mod mb;
>  	struct rtcanmsg *rtcan;
> -	struct nlmsghdr *nlh = nlmsg_put(skb, 0, 0, 0, sizeof(*rtcan), 0);
> +	struct nlmsghdr *nlh;
> +
> +	nlh = nlmsg_put(skb, pid, seq, type, sizeof(*rtcan), flags);
>  	if (!nlh)
>  		return -EMSGSIZE;
>  
> @@ -546,7 +549,8 @@ static int cgw_dump_jobs(struct sk_buff *skb, struct netlink_callback *cb)
>  		if (idx < s_idx)
>  			goto cont;
>  
> -		if (cgw_put_job(skb, gwj) < 0)
> +		if (cgw_put_job(skb, gwj, RTM_NEWROUTE, NETLINK_CB(cb->skb).pid,
> +		    cb->nlh->nlmsg_seq, NLM_F_MULTI) < 0)
>  			break;
>  cont:
>  		idx++;


Hello Thomas,

what about this approach which reduces the number of parameters and stack
space when calling cgw_put_job() ??


diff --git a/net/can/gw.c b/net/can/gw.c
index b41acf2..b6b9e0a 100644
--- a/net/can/gw.c
+++ b/net/can/gw.c
@@ -444,11 +444,15 @@ static int cgw_notifier(struct notifier_block *nb,
 	return NOTIFY_DONE;
 }
 
-static int cgw_put_job(struct sk_buff *skb, struct cgw_job *gwj)
+static int cgw_put_job(struct sk_buff *skb, struct cgw_job *gwj,
+		       struct netlink_callback *cb)
 {
 	struct cgw_frame_mod mb;
 	struct rtcanmsg *rtcan;
-	struct nlmsghdr *nlh = nlmsg_put(skb, 0, 0, 0, sizeof(*rtcan), 0);
+	struct nlmsghdr *nlh;
+
+	nlh = nlmsg_put(skb, NETLINK_CB(cb->skb).pid, cb->nlh->nlmsg_seq,
+			RTM_NEWROUTE, sizeof(*rtcan), NLM_F_MULTI);
 	if (!nlh)
 		return -EMSGSIZE;
 
@@ -571,7 +575,7 @@ static int cgw_dump_jobs(struct sk_buff *skb, struct netlink_callback *cb)
 		if (idx < s_idx)
 			goto cont;
 
-		if (cgw_put_job(skb, gwj) < 0)
+		if (cgw_put_job(skb, gwj, cb) < 0)
 			break;
 cont:
 		idx++;


Regards,
Oliver

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/4] can: Properly fill the netlink header when responding to RTM_GETROUTE
  2012-07-05 12:19 ` [PATCH 3/4] can: Properly fill the netlink header when responding to RTM_GETROUTE Thomas Graf
  2012-07-06 20:39   ` Oliver Hartkopp
@ 2012-07-07  9:18   ` Oliver Hartkopp
  2012-07-10 15:29     ` Marc Kleine-Budde
  1 sibling, 1 reply; 13+ messages in thread
From: Oliver Hartkopp @ 2012-07-07  9:18 UTC (permalink / raw)
  To: Thomas Graf; +Cc: linux-can, Marc Kleine-Budde

Hello Thomas,

besides the fact that i would like to simplify this patch (3/4) as shown
below, i was able to test it now.

You may add my

Tested-by: Oliver Hartkopp <socketcan@hartkopp.net>
Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>

to the next post.

Do you have an URL to pull this patch set for Marc?

Thanks for your effort!

Best regards,
Oliver



On 05.07.2012 14:19, Thomas Graf wrote:

>  - set message type to RTM_NEWROUTE
>  - relate to original request by inheriting the sequence and port number.
>  - set NLM_F_MULTI because it's a dump and more messages will follow
> 
> Signed-off-by: Thomas Graf <tgraf@suug.ch>
> ---
>  net/can/gw.c |   10 +++++++---
>  1 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/net/can/gw.c b/net/can/gw.c
> index a1c639c..20c36e1 100644
> --- a/net/can/gw.c
> +++ b/net/can/gw.c
> @@ -444,11 +444,14 @@ static int cgw_notifier(struct notifier_block *nb,
>  	return NOTIFY_DONE;
>  }
>  
> -static int cgw_put_job(struct sk_buff *skb, struct cgw_job *gwj)
> +static int cgw_put_job(struct sk_buff *skb, struct cgw_job *gwj, int type,
> +		       u32 pid, u32 seq, int flags)
>  {
>  	struct cgw_frame_mod mb;
>  	struct rtcanmsg *rtcan;
> -	struct nlmsghdr *nlh = nlmsg_put(skb, 0, 0, 0, sizeof(*rtcan), 0);
> +	struct nlmsghdr *nlh;
> +
> +	nlh = nlmsg_put(skb, pid, seq, type, sizeof(*rtcan), flags);
>  	if (!nlh)
>  		return -EMSGSIZE;
>  
> @@ -546,7 +549,8 @@ static int cgw_dump_jobs(struct sk_buff *skb, struct netlink_callback *cb)
>  		if (idx < s_idx)
>  			goto cont;
>  
> -		if (cgw_put_job(skb, gwj) < 0)
> +		if (cgw_put_job(skb, gwj, RTM_NEWROUTE, NETLINK_CB(cb->skb).pid,
> +		    cb->nlh->nlmsg_seq, NLM_F_MULTI) < 0)
>  			break;
>  cont:
>  		idx++;




Hello Thomas,

what about this approach which reduces the number of parameters and stack
space when calling cgw_put_job() ??


diff --git a/net/can/gw.c b/net/can/gw.c
index b41acf2..b6b9e0a 100644
--- a/net/can/gw.c
+++ b/net/can/gw.c
@@ -444,11 +444,15 @@ static int cgw_notifier(struct notifier_block *nb,
 	return NOTIFY_DONE;
 }
-static int cgw_put_job(struct sk_buff *skb, struct cgw_job *gwj)
+static int cgw_put_job(struct sk_buff *skb, struct cgw_job *gwj,
+		       struct netlink_callback *cb)
 {
 	struct cgw_frame_mod mb;
 	struct rtcanmsg *rtcan;
-	struct nlmsghdr *nlh = nlmsg_put(skb, 0, 0, 0, sizeof(*rtcan), 0);
+	struct nlmsghdr *nlh;
+
+	nlh = nlmsg_put(skb, NETLINK_CB(cb->skb).pid, cb->nlh->nlmsg_seq,
+			RTM_NEWROUTE, sizeof(*rtcan), NLM_F_MULTI);
 	if (!nlh)
 		return -EMSGSIZE;
@@ -571,7 +575,7 @@ static int cgw_dump_jobs(struct sk_buff *skb, struct netlink_callback *cb)
 		if (idx < s_idx)
 			goto cont;
-		if (cgw_put_job(skb, gwj) < 0)
+		if (cgw_put_job(skb, gwj, cb) < 0)
 			break;
 cont:
 		idx++;


Regards,
Oliver

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/4] can: Properly fill the netlink header when responding to RTM_GETROUTE
  2012-07-06 20:39   ` Oliver Hartkopp
@ 2012-07-09  9:43     ` Thomas Graf
  2012-07-09 16:28       ` Oliver Hartkopp
  0 siblings, 1 reply; 13+ messages in thread
From: Thomas Graf @ 2012-07-09  9:43 UTC (permalink / raw)
  To: Oliver Hartkopp; +Cc: linux-can

On Fri, Jul 06, 2012 at 10:39:05PM +0200, Oliver Hartkopp wrote:
> what about this approach which reduces the number of parameters and stack
> space when calling cgw_put_job() ??
> 
> 
> diff --git a/net/can/gw.c b/net/can/gw.c
> index b41acf2..b6b9e0a 100644
> --- a/net/can/gw.c
> +++ b/net/can/gw.c
> @@ -444,11 +444,15 @@ static int cgw_notifier(struct notifier_block *nb,
>  	return NOTIFY_DONE;
>  }
>  
> -static int cgw_put_job(struct sk_buff *skb, struct cgw_job *gwj)
> +static int cgw_put_job(struct sk_buff *skb, struct cgw_job *gwj,
> +		       struct netlink_callback *cb)
>  {
>  	struct cgw_frame_mod mb;
>  	struct rtcanmsg *rtcan;
> -	struct nlmsghdr *nlh = nlmsg_put(skb, 0, 0, 0, sizeof(*rtcan), 0);
> +	struct nlmsghdr *nlh;
> +
> +	nlh = nlmsg_put(skb, NETLINK_CB(cb->skb).pid, cb->nlh->nlmsg_seq,
> +			RTM_NEWROUTE, sizeof(*rtcan), NLM_F_MULTI);
>  	if (!nlh)
>  		return -EMSGSIZE;
>  
> @@ -571,7 +575,7 @@ static int cgw_dump_jobs(struct sk_buff *skb, struct netlink_callback *cb)
>  		if (idx < s_idx)
>  			goto cont;
>  
> -		if (cgw_put_job(skb, gwj) < 0)
> +		if (cgw_put_job(skb, gwj, cb) < 0)
>  			break;
>  cont:
>  		idx++;

The format I've chosen allows to implement a non-dump RTM_GETROUTE using
cgw_put_job() if needed and is in line with how such filling functions
are typically prototyped in other netlink protocol implementations.

~Thomas

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/4] can: Properly fill the netlink header when responding to RTM_GETROUTE
  2012-07-09  9:43     ` Thomas Graf
@ 2012-07-09 16:28       ` Oliver Hartkopp
  0 siblings, 0 replies; 13+ messages in thread
From: Oliver Hartkopp @ 2012-07-09 16:28 UTC (permalink / raw)
  To: Thomas Graf; +Cc: linux-can

On 09.07.2012 11:43, Thomas Graf wrote:

> On Fri, Jul 06, 2012 at 10:39:05PM +0200, Oliver Hartkopp wrote:
>> what about this approach which reduces the number of parameters and stack
>> space when calling cgw_put_job() ??
>>
>>
>> diff --git a/net/can/gw.c b/net/can/gw.c
>> index b41acf2..b6b9e0a 100644
>> --- a/net/can/gw.c
>> +++ b/net/can/gw.c
>> @@ -444,11 +444,15 @@ static int cgw_notifier(struct notifier_block *nb,
>>  	return NOTIFY_DONE;
>>  }
>>  
>> -static int cgw_put_job(struct sk_buff *skb, struct cgw_job *gwj)
>> +static int cgw_put_job(struct sk_buff *skb, struct cgw_job *gwj,
>> +		       struct netlink_callback *cb)
>>  {
>>  	struct cgw_frame_mod mb;
>>  	struct rtcanmsg *rtcan;
>> -	struct nlmsghdr *nlh = nlmsg_put(skb, 0, 0, 0, sizeof(*rtcan), 0);
>> +	struct nlmsghdr *nlh;
>> +
>> +	nlh = nlmsg_put(skb, NETLINK_CB(cb->skb).pid, cb->nlh->nlmsg_seq,
>> +			RTM_NEWROUTE, sizeof(*rtcan), NLM_F_MULTI);
>>  	if (!nlh)
>>  		return -EMSGSIZE;
>>  
>> @@ -571,7 +575,7 @@ static int cgw_dump_jobs(struct sk_buff *skb, struct netlink_callback *cb)
>>  		if (idx < s_idx)
>>  			goto cont;
>>  
>> -		if (cgw_put_job(skb, gwj) < 0)
>> +		if (cgw_put_job(skb, gwj, cb) < 0)
>>  			break;
>>  cont:
>>  		idx++;
> 
> The format I've chosen allows to implement a non-dump RTM_GETROUTE using
> cgw_put_job() if needed and is in line with how such filling functions
> are typically prototyped in other netlink protocol implementations.
> 


Yes, i also checked that the format was commonly used, but i did not find any
use-case for cgw_put_job() besides the full dump ...

Btw. both approaches are fine to me then.

Many thanks,
Oliver


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/4] can: Properly fill the netlink header when responding to RTM_GETROUTE
  2012-07-07  9:18   ` Oliver Hartkopp
@ 2012-07-10 15:29     ` Marc Kleine-Budde
  2012-07-10 18:48       ` Oliver Hartkopp
  0 siblings, 1 reply; 13+ messages in thread
From: Marc Kleine-Budde @ 2012-07-10 15:29 UTC (permalink / raw)
  To: Oliver Hartkopp; +Cc: Thomas Graf, linux-can

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

On 07/07/2012 11:18 AM, Oliver Hartkopp wrote:
> Hello Thomas,
> 
> besides the fact that i would like to simplify this patch (3/4) as shown
> below, i was able to test it now.
> 
> You may add my
> 
> Tested-by: Oliver Hartkopp <socketcan@hartkopp.net>
> Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>
> 
> to the next post.
> 
> Do you have an URL to pull this patch set for Marc?

I can take this series as is and add your Tested-by and Acked-by to each
patch. Is this okay?

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/4] can: Properly fill the netlink header when responding to RTM_GETROUTE
  2012-07-10 15:29     ` Marc Kleine-Budde
@ 2012-07-10 18:48       ` Oliver Hartkopp
  0 siblings, 0 replies; 13+ messages in thread
From: Oliver Hartkopp @ 2012-07-10 18:48 UTC (permalink / raw)
  To: Marc Kleine-Budde; +Cc: Thomas Graf, linux-can

On 10.07.2012 17:29, Marc Kleine-Budde wrote:

> On 07/07/2012 11:18 AM, Oliver Hartkopp wrote:
>> Hello Thomas,
>>
>> besides the fact that i would like to simplify this patch (3/4) as shown
>> below, i was able to test it now.
>>
>> You may add my
>>
>> Tested-by: Oliver Hartkopp <socketcan@hartkopp.net>
>> Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>
>>
>> to the next post.
>>
>> Do you have an URL to pull this patch set for Marc?
> 
> I can take this series as is and add your Tested-by and Acked-by to each
> patch. Is this okay?


Yes. That would be fine!

Tnx,
Oliver


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/4] can: gw: Netlink related fixes
  2012-07-05 12:19 [PATCH 0/4] can: gw: Netlink related fixes Thomas Graf
                   ` (4 preceding siblings ...)
  2012-07-05 16:16 ` [PATCH 0/4] can: gw: Netlink related fixes Oliver Hartkopp
@ 2012-07-10 20:55 ` Marc Kleine-Budde
  5 siblings, 0 replies; 13+ messages in thread
From: Marc Kleine-Budde @ 2012-07-10 20:55 UTC (permalink / raw)
  To: Thomas Graf; +Cc: linux-can

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

On 07/05/2012 02:19 PM, Thomas Graf wrote:
> Hi
> 
> These patches fix some Netlink quirks in the CAN gateway code.
> Unfortunately I do not own any CAN hardware so this is compile
> tested only. I'm fairly confident though as the changes are
> mostly straight forward. I would appreciate if someone could
> run them through a test suite or something though.
> 
> 
> Thomas Graf (4):
>   can: Don't bump nlmsg_len manually
>   can: Use nla_policy to validate netlink attributes
>   can: Properly fill the netlink header when responding to RTM_GETROUTE
>   can: Remove pointless casts

Thanks applied to linux-can-next.

Thanks, Marc
-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2012-07-10 20:55 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-07-05 12:19 [PATCH 0/4] can: gw: Netlink related fixes Thomas Graf
2012-07-05 12:19 ` [PATCH 1/4] can: Don't bump nlmsg_len manually Thomas Graf
2012-07-05 12:19 ` [PATCH 2/4] can: Use nla_policy to validate netlink attributes Thomas Graf
2012-07-05 12:19 ` [PATCH 3/4] can: Properly fill the netlink header when responding to RTM_GETROUTE Thomas Graf
2012-07-06 20:39   ` Oliver Hartkopp
2012-07-09  9:43     ` Thomas Graf
2012-07-09 16:28       ` Oliver Hartkopp
2012-07-07  9:18   ` Oliver Hartkopp
2012-07-10 15:29     ` Marc Kleine-Budde
2012-07-10 18:48       ` Oliver Hartkopp
2012-07-05 12:19 ` [PATCH 4/4] can: Remove pointless casts Thomas Graf
2012-07-05 16:16 ` [PATCH 0/4] can: gw: Netlink related fixes Oliver Hartkopp
2012-07-10 20:55 ` Marc Kleine-Budde

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.