[B.A.T.M.A.N.] [PATCH 1/2] batman-adv: count_real_packets() in batman-adv assumes char is signed
count_real_packets() in batman-adv assumes char is signed, and returns -1 through it:
net/batman-adv/routing.c: In function 'receive_bat_packet': net/batman-adv/routing.c:739: warning: comparison is always false due to limited range of data type
Use int instead.
This is also looks a bit weird as (presumably signed) is_duplicate is constructed by OR'ding together the unsigned results of get_bit_status() (though the latter only returns 0 or 1).
Signed-off-by: David Howells dhowells@redhat.com cc: Marek Lindner lindner_marek@yahoo.de cc: Simon Wunderlich siwu@hrz.tu-chemnitz.de cc: Sven Eckelmann sven@narfation.org cc: b.a.t.m.a.n@lists.open-mesh.org cc: netdev@vger.kernel.org ---
net/batman-adv/routing.c | 10 +++++----- 1 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/net/batman-adv/routing.c b/net/batman-adv/routing.c index bb1c3ec..3075fcb 100644 --- a/net/batman-adv/routing.c +++ b/net/batman-adv/routing.c @@ -531,15 +531,15 @@ static int window_protected(struct bat_priv *bat_priv, * -1 the packet is old and has been received while the seqno window * was protected. Caller should drop it. */ -static char count_real_packets(struct ethhdr *ethhdr, - struct batman_packet *batman_packet, - struct hard_iface *if_incoming) +static int count_real_packets(struct ethhdr *ethhdr, + struct batman_packet *batman_packet, + struct hard_iface *if_incoming) { struct bat_priv *bat_priv = netdev_priv(if_incoming->soft_iface); struct orig_node *orig_node; struct neigh_node *tmp_neigh_node; struct hlist_node *node; - char is_duplicate = 0; + uint8_t is_duplicate = 0; int32_t seq_diff; int need_update = 0; int set_mark, ret = -1; @@ -608,7 +608,7 @@ void receive_bat_packet(struct ethhdr *ethhdr, char has_directlink_flag; char is_my_addr = 0, is_my_orig = 0, is_my_oldorig = 0; char is_broadcast = 0, is_bidirectional, is_single_hop_neigh; - char is_duplicate; + int is_duplicate; uint32_t if_incoming_seqno;
/* Silently drop when the batman packet is actually not a
compare_eth() should take const pointer arguments so that it can be passed const pointers without the need for a cast, leading to:
net/batman-adv/vis.c: In function 'vis_data_insert_interface': net/batman-adv/vis.c:146: warning: passing argument 2 of 'compare_eth' discards qualifiers from pointer target type
Signed-off-by: David Howells dhowells@redhat.com cc: Marek Lindner lindner_marek@yahoo.de cc: Simon Wunderlich siwu@hrz.tu-chemnitz.de cc: Sven Eckelmann sven@narfation.org cc: b.a.t.m.a.n@lists.open-mesh.org cc: netdev@vger.kernel.org ---
net/batman-adv/main.h | 2 +- net/batman-adv/vis.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/batman-adv/main.h b/net/batman-adv/main.h index 148b49e..e6fc798 100644 --- a/net/batman-adv/main.h +++ b/net/batman-adv/main.h @@ -172,7 +172,7 @@ static inline void bat_dbg(char type __always_unused, * * note: can't use compare_ether_addr() as it requires aligned memory */ -static inline int compare_eth(void *data1, void *data2) +static inline int compare_eth(const void *data1, const void *data2) { return (memcmp(data1, data2, ETH_ALEN) == 0 ? 1 : 0); } diff --git a/net/batman-adv/vis.c b/net/batman-adv/vis.c index c39f20c..34053ac 100644 --- a/net/batman-adv/vis.c +++ b/net/batman-adv/vis.c @@ -143,7 +143,7 @@ static void vis_data_insert_interface(const uint8_t *interface, struct hlist_node *pos;
hlist_for_each_entry(entry, pos, if_list, list) { - if (compare_eth(entry->addr, (void *)interface)) + if (compare_eth(entry->addr, interface)) return; }
David Howells wrote:
compare_eth() should take const pointer arguments so that it can be passed const pointers without the need for a cast, leading to:
net/batman-adv/vis.c: In function 'vis_data_insert_interface': net/batman-adv/vis.c:146: warning: passing argument 2 of 'compare_eth' discards qualifiers from pointer target type
Signed-off-by: David Howells dhowells@redhat.com cc: Marek Lindner lindner_marek@yahoo.de cc: Simon Wunderlich siwu@hrz.tu-chemnitz.de cc: Sven Eckelmann sven@narfation.org cc: b.a.t.m.a.n@lists.open-mesh.org cc: netdev@vger.kernel.org
net/batman-adv/main.h | 2 +- net/batman-adv/vis.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/batman-adv/main.h b/net/batman-adv/main.h index 148b49e..e6fc798 100644 --- a/net/batman-adv/main.h +++ b/net/batman-adv/main.h @@ -172,7 +172,7 @@ static inline void bat_dbg(char type __always_unused,
- note: can't use compare_ether_addr() as it requires aligned memory
*/ -static inline int compare_eth(void *data1, void *data2) +static inline int compare_eth(const void *data1, const void *data2) { return (memcmp(data1, data2, ETH_ALEN) == 0 ? 1 : 0); } diff --git a/net/batman-adv/vis.c b/net/batman-adv/vis.c index c39f20c..34053ac 100644 --- a/net/batman-adv/vis.c +++ b/net/batman-adv/vis.c @@ -143,7 +143,7 @@ static void vis_data_insert_interface(const uint8_t *interface, struct hlist_node *pos;
hlist_for_each_entry(entry, pos, if_list, list) {
if (compare_eth(entry->addr, (void *)interface))
if (compare_eth(entry->addr, interface)) return;
Sry, but this patch doesn't apply here (net-next-2.6/linux-next)
Kind regards, Sven
David Howells wrote:
count_real_packets() in batman-adv assumes char is signed, and returns -1 through it:
net/batman-adv/routing.c: In function 'receive_bat_packet': net/batman-adv/routing.c:739: warning: comparison is always false due to limited range of data type
Use int instead.
[...]
-static char count_real_packets(struct ethhdr *ethhdr,
struct batman_packet *batman_packet,struct hard_iface *if_incoming)+static int count_real_packets(struct ethhdr *ethhdr,
struct batman_packet *batman_packet,struct hard_iface *if_incoming){
This one doesn't apply on linux-next/net-next-2.6, but I will fix it by hand.
Thanks, Sven
On Wednesday 15 June 2011 01:51:32 David Howells wrote:
count_real_packets() in batman-adv assumes char is signed, and returns -1 through it:
net/batman-adv/routing.c: In function 'receive_bat_packet': net/batman-adv/routing.c:739: warning: comparison is always false due to limited range of data type
Use int instead.
This is also looks a bit weird as (presumably signed) is_duplicate is constructed by OR'ding together the unsigned results of get_bit_status() (though the latter only returns 0 or 1).
Sry, had to catch the train and had no time to explain it further.
It is correct that is_duplicate will only have 0 and 1 stored, but the window_protected function (called before the loop) may detect that the packet has to be dropped and we return in that case -1.
I don't know who started to use char in those places, but thanks for reminding me how much I hate it and that I wanted to check the rest of the code. :)
I will submit the corrected patch in a pull request later this week to David S. Miller.
Thanks, Sven
b.a.t.m.a.n@lists.open-mesh.org
-
David Howells -
Sven Eckelmann