From: Antonio Quartulli <ordex@autistici.org>
To: The list for a Better Approach To Mobile Ad-hoc Networking
<b.a.t.m.a.n@lists.open-mesh.org>
Subject: Re: [B.A.T.M.A.N.] [PATCH 2/4] batman-adv: improved client announcement mechanismy
Date: Sun, 1 May 2011 13:35:37 +0200 [thread overview]
Message-ID: <20110501113536.GA8915@ritirata.org> (raw)
In-Reply-To: <20110430174839.GC6538@lunn.ch>
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
next prev parent reply other threads:[~2011-05-01 11:35 UTC|newest]
Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-04-27 21:35 [B.A.T.M.A.N.] [PATCH] New translation table announcement mechanism patchset Antonio Quartulli
2011-04-27 21:35 ` [B.A.T.M.A.N.] [PATCH 1/4] batman-adv: rename everything from *hna* into *tt* (translation table) Antonio Quartulli
2011-04-27 21:35 ` [B.A.T.M.A.N.] [PATCH 2/4] batman-adv: improved client announcement mechanism Antonio Quartulli
2011-04-28 16:10 ` Andrew Lunn
2011-04-28 16:14 ` Sven Eckelmann
2011-04-28 17:34 ` Marek Lindner
2011-04-28 17:45 ` Antonio Quartulli
2011-04-28 17:46 ` Gioacchino Mazzurco
2011-04-30 8:42 ` [B.A.T.M.A.N.] [PATCH 2/4] batman-adv: improved client announcement mechanismy Andrew Lunn
2011-04-30 16:00 ` Antonio Quartulli
2011-04-30 17:48 ` Andrew Lunn
2011-05-01 11:35 ` Antonio Quartulli [this message]
2011-05-01 13:10 ` [B.A.T.M.A.N.] [PATCH 2/4] batman-adv: improved client announcement mechanism Andrew Lunn
2011-05-03 15:50 ` [B.A.T.M.A.N.] [PATCH 2/4] batman-adv: improved client announcement mechanismy Antonio Quartulli
2011-05-03 15:56 ` Marek Lindner
2011-05-03 17:07 ` Antonio Quartulli
2011-04-27 21:35 ` [B.A.T.M.A.N.] [PATCH 3/4] batman-adv: improved roaming mechanism Antonio Quartulli
2011-04-27 21:35 ` [B.A.T.M.A.N.] [PATCH 4/4] batman-adv: protect the local and the global trans-tables with rcu Antonio Quartulli
2011-05-03 15:54 ` [B.A.T.M.A.N.] New translation table announcement mechanism patchset v2 Antonio Quartulli
2011-05-03 15:54 ` [B.A.T.M.A.N.] [PATCHv2 1/4] batman-adv: rename everything from *hna* into *tt* (translation table) Antonio Quartulli
2011-05-05 6:42 ` [B.A.T.M.A.N.] [PATCHv3] " Antonio Quartulli
2011-05-07 17:46 ` Marek Lindner
2011-05-03 15:54 ` [B.A.T.M.A.N.] [PATCHv2 2/4] batman-adv: improved client announcement mechanism Antonio Quartulli
2011-05-03 15:54 ` [B.A.T.M.A.N.] [PATCHv2 3/4] batman-adv: improved roaming mechanism Antonio Quartulli
2011-05-04 11:22 ` Andrew Lunn
2011-05-04 13:36 ` Antonio Quartulli
2011-05-04 13:52 ` Andrew Lunn
2011-05-04 13:59 ` Antonio Quartulli
2011-05-03 15:54 ` [B.A.T.M.A.N.] [PATCHv2 4/4] batman-adv: protect the local and the global trans-tables with rcu Antonio Quartulli
2011-05-09 9:49 ` [B.A.T.M.A.N.] [PATCHv3 1/3] batman-adv: improved client announcement mechanism Antonio Quartulli
2011-05-09 9:49 ` [B.A.T.M.A.N.] [PATCHv3 2/3] batman-adv: improved roaming mechanism Antonio Quartulli
2011-05-09 9:49 ` [B.A.T.M.A.N.] [PATCH 3/3] batman-adv: protect the local and the global trans-tables with rcu Antonio Quartulli
2011-05-09 9:50 ` [B.A.T.M.A.N.] [PATCHv3 " Antonio Quartulli
2011-05-10 13:02 ` [B.A.T.M.A.N.] [PATCHv4 1/3] batman-adv: improved client announcement mechanism Antonio Quartulli
2011-05-17 18:53 ` Simon Wunderlich
2011-05-17 19:09 ` Sven Eckelmann
2011-05-17 19:22 ` Sven Eckelmann
2011-05-10 13:02 ` [B.A.T.M.A.N.] [PATCHv4 2/3] batman-adv: improved roaming mechanism Antonio Quartulli
2011-05-10 13:02 ` [B.A.T.M.A.N.] [PATCHv4 3/3] batman-adv: protect the local and the global trans-tables with rcu Antonio Quartulli
2011-05-21 21:25 ` [B.A.T.M.A.N.] [PATCHv5 0/3] New translation table announcement mechanism patchset v5 Antonio Quartulli
2011-05-21 21:25 ` [B.A.T.M.A.N.] [PATCHv5 1/3] batman-adv: improved client announcement mechanism Antonio Quartulli
2011-05-22 11:01 ` Sven Eckelmann
2011-05-23 13:23 ` Antonio Quartulli
2011-05-21 21:25 ` [B.A.T.M.A.N.] [PATCHv2 2/3] batman-adv: improved roaming mechanism Antonio Quartulli
2011-05-22 11:01 ` Sven Eckelmann
2011-05-22 11:44 ` Sven Eckelmann
2011-05-21 21:25 ` [B.A.T.M.A.N.] [PATCHv5 3/3] batman-adv: protect the local and the global trans-tables with rcu Antonio Quartulli
2011-05-28 17:29 ` [B.A.T.M.A.N.] [PATCH] New translation table announcement mechanism patchset Marek Lindner
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20110501113536.GA8915@ritirata.org \
--to=ordex@autistici.org \
--cc=b.a.t.m.a.n@lists.open-mesh.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).