Re: [B.A.T.M.A.N.] [PATCH 2/4] batman-adv: improved client announcement mechanismy

[Antonio Quartulli <ordex@autistici.org>]

On Sat, Apr 30, 2011 at 07:48:39PM +0200, Andrew Lunn wrote:
> > > You indentation/wrapping is a bit strange. In the function
> > > declaration, i would of put the int tt_num_changes directly under int
> > > buff_pos.
> > 
> > This is what I've done, but it seems that your mail client is messing up
> > with the tabs (I think).
> 
> Possibly. Or the list server. I use mutt, same as you, and it normally
> gets tabs and the like correct.
> 
Yes..then I don't know :)

> > > > +++ b/packet.h
> > > > @@ -30,9 +30,10 @@
> > > >  #define BAT_BCAST        0x04
> > > >  #define BAT_VIS          0x05
> > > >  #define BAT_UNICAST_FRAG 0x06
> > > > +#define BAT_TT_QUERY	 0x07
> > > 
> > > Indentation?
> > 
> > As above, but in this case I think I'll substitute the tab with spaces so
> > that all the BAT_* definitions can be homogeneous
> 
> checkpatch might complain, depending on the number of spaces.

Yep, I'll keep the patch checkpatch-compilant ;)

>  
> > > > +/* TT flags */
> > > > +#define TT_RESPONSE	0x00
> > > > +#define TT_REQUEST	0x01
> > > > +#define TT_FULL_TABLE	0x02
> > > > +
> > > > @@ -101,6 +109,7 @@ struct unicast_packet {
> > > >  	uint8_t  version;  /* batman version field */
> > > >  	uint8_t  dest[6];
> > > >  	uint8_t  ttl;
> > > > +	uint8_t  ttvn; /* destination ttvn */
> > > >  } __packed;
> > > 
> > > What is ttvn? The vn in particular? Is it version? There is already
> > > ver and version used, do we want yet another way to say version?
> > > 
> > 
> > Translation Table Version Number. 'ttvn' is the abbreviation used in the
> > documentation, so I decided to use it as field name. Only in the struct
> > orig_node it is called last_tt_ver_num. Do you think I should use the
> > latter everywhere? 'ttvn' is really nice and compact :)
> 
> It is a minor point. ttvn is O.K. But how about ttver?

Mh..Honestly I like ttvn, but I can put and explicit explanation in the
field declaration in types.h.

I would also like to know what the other people think about

> > > Rather than document the bits, it would be better to reference to the
> > > macros TT_*. Somebody at some time with add new flags, or change the
> > > values and not update this description.
> > 
> > Mh..Honestly I prefer to understand what each bit means in a bitfield
> > flag. What do you mean with reference to the macros? 
> 
> I mean say that flags is a combination of TT_RESPONSE, TT_REQUEST,
> TT_FULL_TABLE. The TT_* macros.

Oky!

> > > > +	uint16_t tt_data;	/* if tt_request: crc associated with the
> > > > +				 *		   ttvn
> > > > +				 * if tt_response: table_size
> > > > +				 */
> > > 
> > > Maybe a union instead of tt_data being used for two different things?
> > > Makes it less confusing when reading the code.
> > 
> > I decided to avoid a union because here we have two different things
> > which have exactly the same length. So I opted for a "generic" name.
> > What do style experts suggest? :)
> > A union would probably make easier to understand what is going on while
> > reading the code, as Andrew suggested.
> 
> I believe in the saying: Code is written once, read a 100 times, so
> make it easy to read.
> 


> > > > +	/* the ttvn increased by one -> we can apply the attached changes */
> > > > +	if (ttvn - orig_ttvn == 1) {
> > > > +		/* if it does not contain the changes send a tt request */
> > > > +		if (!tt_num_changes)
> > > > +			goto request_table;
> > > 
> > > Why would that happen? It sounds like you are handling a bug, not
> > > something which is designed to happen. 
> > >
> > 
> > We have two cases which would lead to this situation:
> > 1) An OGM, after being sent TT_OGM_APPEND_MAX times, will not contain
> > the changes anymore. If a node missed all the "full" OGMs, it will
> > end up in this situation when receiving the next one.
> > 2) The set of changes is too big to be appended to the OGM (due to the frame
> > maximum size). The receiving node will send a tt_request to recover the
> > changes (later on, it could also exploit the fragmentation, while the
> > OGM cannot)
> 
> O.K. so there is a good reason. So maybe hint about these reasons in
> the comment? Help the reader understand why it might happen.

Ok I can add some comments more. But, should I reason as we do not have
documentation at all? I mean, while deciding to put a comment or not..

Because, in my opinion, this piece of code woule be clearer after reading the
doc.

> 
> > Yes, memory problem. Actually it is not possible to make two passes:
> > e.g. imagine that the set of changes is the following:
> > - DEL A
> > - ADD A
> > - DEL A
> > (ok it is probably not really common, but still possible)
> 
> And since it will not happen to often, it is not worth the code so
> find such situations and simply the transactions.
> 

What do you exactly mean? Sorry but I didn't fully understand your
sentence :(

> > Remember that we added the TT_CRC. It was born as conutermeasure to node
> > reboots, but now we are exploiting it as consistency check! This is why
> > the code recomputes the crc after applying every change set. If something
> > went wrong, on the next OGM the node will recognise the problem and ask
> > for a "full table".
> 
> O.K. a clean self recovery. That is good.

;)

> 
> > > > +	if (tt_query->flags & TT_REQUEST) {
> > > > +		/* Try to reply to this tt_request */
> > > > +		ret = send_tt_response(bat_priv, tt_query);
> > > > +		if (ret != NET_RX_SUCCESS) {
> > > 
> > > This looks wrong. The name send_tt_response() suggests we are sending,
> > > but you compare against NET_RX_SUCCESS!
> > 
> > eheh you're nearly right. We are sending a tt_response here, BUT only if we
> > have enough information to send such message we can consider the
> > tt_request as correctly received, otherwise, as the code below suggests,
> > we have to re-route the packet and so let route_unicast_packet() decide
> > whether the mesage was correctly received or not.
> 
> You definitely need a comment here. It is so counter intuitive that
> you are bound to get bug reports and patches by people who see this.

Ok, I'll add a commente here too

> 
> > > > +			bat_dbg(DBG_ROUTES, bat_priv,
> > > > +				"Routing TT_REQUEST to %pM [%c]\n",
> > > > +				tt_query->dst,
> > > > +				(tt_query->flags & TT_FULL_TABLE ? 'F' : '.'));
> > > > +			tt_query->tt_data = htons(tt_query->tt_data);
> > > > +			return route_unicast_packet(skb, recv_if);
> > > > +		}
> > > > +		goto out;
> > > > +	}
> > > > +	/* We need to linearize the packet to access the TT data */
> > > > +	if (skb_linearize(skb) < 0)
> > > > +		goto out;
> > > 
> > > Isn't this too late. You have already accessed tt_query->tt_data in
> > > the code above?
> > > 
> > 
> > the access to the tt_data field is guaranteed by 
> >
> > pskb_may_pull(skb, sizeof(struct tt_query_packet))
> > 
> > (a few lines above inside the function), while here we are linearising the
> > skb to let handle_tt_reponse access the data contained after the header.
> 
> Ah, O.K. The comment is ambiguous and i got the wrong meaning. Maybe
> the comment could be:
> 
> 	/* We need to linearize the packet to access the TT change records */
> 

Oky I'll correct the comment :-)


I understood that I have to work harder to write comments :D
Thank you again!


Regards,

-- 
Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara