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

[Andrew Lunn <andrew@lunn.ch>]

> > 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.

> > > +++ 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. 
 
> > > +/* 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? 

> 
> 
> > > @@ -134,4 +143,25 @@ struct vis_packet {
> > >  	uint8_t  sender_orig[6]; /* who sent or rebroadcasted this packet */
> > >  } __packed;
> > >  
> > > +struct tt_query_packet {
> > > +	uint8_t  packet_type;
> > > +	uint8_t  version;  /* batman version field */
> > > +	uint8_t  dst[6];
> > > +	uint8_t  ttl;
> > > +	uint8_t  flags;		/* bit0: 0: -> tt_request
> > > +				 *	 1: -> tt_response
> > > +				 * bit1: request the full table
> > > +				 */
> > 
> > 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.

> > > +	uint8_t  src[6];
> > > +	uint8_t  ttvn;		/* if tt_request: ttvn that triggered the
> > > +				 *		  request
> > > +				 * if tt_response: new ttvn for the src
> > > +				 * orig_node
> > > +				 */
> > > +	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.

> 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.

> 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. 

> > > +			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 */

	Andrew