On Sunday, September 01, 2013 02:38:29 Antonio Quartulli wrote:
diff --git a/hard-interface.c b/hard-interface.c index c5f871f..c60d3ed 100644 --- a/hard-interface.c +++ b/hard-interface.c @@ -266,16 +266,9 @@ static void batadv_check_known_mac_addr(const struct net_device *net_dev)
int batadv_hardif_min_mtu(struct net_device *soft_iface) {
- const struct batadv_priv *bat_priv = netdev_priv(soft_iface);
- struct batadv_priv *bat_priv = netdev_priv(soft_iface);
Why are you removing the const qualifier?
CC [M] /home/marek/batman-adv/hard-interface.o /home/marek/batman-adv/hard-interface.c: In function ‘batadv_hardif_min_mtu’: /home/marek/batman-adv/hard-interface.c:286:2: warning: passing argument 1 of ‘atomic_set’ discards ‘const’ qualifier from pointer target type [enabled by default] /usr/src/linux-headers-3.2.0-4-common/arch/x86/include/asm/atomic.h:35:20: note: expected ‘struct atomic_t *’ but argument is of type ‘const struct atomic_t *’ /home/marek/batman-adv/hard-interface.c:298:2: warning: passing argument 1 of ‘atomic_set’ discards ‘const’ qualifier from pointer target type [enabled by default] /usr/src/linux-headers-3.2.0-4-common/arch/x86/include/asm/atomic.h:35:20: note: expected ‘struct atomic_t *’ but argument is of type ‘const struct atomic_t *’
/* Register the client MAC in the transtable */
- if (!is_multicast_ether_addr(ethhdr->h_source))
batadv_tt_local_add(soft_iface, ethhdr->h_source, vid,skb->skb_iif);
- if (!is_multicast_ether_addr(ethhdr->h_source)) {
client_added = batadv_tt_local_add(soft_iface, ethhdr->h_source,vid, skb->skb_iif);Isn't this assignment as ugly as the following if condition?
if (!batadv_tt_local_add(soft_iface, ethhdr->h_source, vid, skb->skb_iif))client_added looks useless to me.
The general idea behind "client_added" was to increase readibility. For the casual reader this variable makes it more obvious what goes on.
+/**
- batadv_tt_local_table_transmit_size - calculates the local
translation table + * size when transmitted over the air
- @bat_priv: the bat priv with all the soft interface information
- Returns local translation table size in bytes.
- */
+static int batadv_tt_local_table_transmit_size(struct batadv_priv *bat_priv) +{
- struct batadv_softif_vlan *vlan;
- uint16_t num_vlan = 0, tt_local_entries = 0;
- int hdr_size;
David always asks to sort variable declaration lines from the longest to the shorter. IMHO it is not really important (in particular in this case) but since you are creating the function now, please follow that indication.
Sure.
@@ -717,12 +762,6 @@ static void batadv_tt_tvlv_container_update(struct batadv_priv *bat_priv)
tt_diff_entries_num = atomic_read(&bat_priv->tt.local_changes); tt_diff_len = batadv_tt_len(tt_diff_entries_num);
- /* if we have too many changes for one packet don't send any
* and wait for the tt table request which will be fragmented*/- if (tt_diff_len > bat_priv->soft_iface->mtu)
tt_diff_len = 0;Why are you removing this? We are not going to fragment OGMs and in this function we are preparing the TVLV to send within one of those.
Upon reading this function a realized this "tt_diff_len = 0" mechanism is buggy. For instance, the changes_list list is not purged but keeps growing until it magically fits the ogm diff again. Or checking bat_priv->soft_iface-
mtu isn't terribly accurate either. The more I looked the more my head
started to spin. Suddenly, I was convinced we don't need the check anymore because we have the fragmentation but you are right - it won't help here. I'll add the check again but be aware that it is broken.
Thanks for the review!
Cheers, Marek