On Wed, 21 Nov 2018 at 12:45, Antony Antony <[email protected]> wrote: > > On Wed, Nov 21, 2018 at 10:44:14AM -0500, Andrew Cagney wrote: > > Yea, two changes: > > > > - move the function to demux.[hc] > > The function, while currently IKEv2 specific, belongs in demux.[hc] as > > it operates on the msg_digest; and like the commit points out, several > > strange #includes were eliminated > > > > - rename v2_msg_role() to msg_role() > > This one I umed and arhd over a bit. It makes code like: > > switch (msg_role(md)) { > > case MESSAGE_REQUEST: ... > > slightly less verbose. However, to your point, it seems that only > > IKEv2 can easily figure out if a message is a request or a response > > (which really scares me). Given this, should > > MESSAGE_{REQUEST,RESPONSE} also get the V2_* treatment vis: > > switch(v2_msg_role(md)) { > > case V2_MESSAGE_REQUEST: ... > > but that is really looking cumbersome. > > I am confused about these series of changes. 3 concerns comes to mind about > v2_msg_role and varients. > > 1. Me like ikev2_ (firs prefix) function names. Just v2 is confusing to me. > It could be v2 of something else. nss.. also v2 preix appears at various > positions. Some times as the first prefix, other times in the middle. Just > some functions we have v2 versions. However, this has been going on for > while, possibly too late to change.
I believe the, er, guidance we were given was to use the shorter v[12]. > 2 is_msg_request() in ikev2.c seems very close to the new msg_role? > There are too much git moving around to track them down. I can't tell what > is the intention here. md's original_role was replaced by .message_role (but that introduced duplicate information so I turned it into a function). While both is_msg_request() and is_msg_response() have become redundant, replacing them with msg_role() isn't necessarily correct - .st_sa_role is typically better but that requires care (as we've seen from a recent commit of mine). > 3. if msg_role() is in demux.c it should be ikev2_msg_role() > > While quibbling I have another one. > Andrew is lately adding a bunch variables and function names with same name. > It is annoying to read:) It makes hard to find definations, and declerations > using cscope or grep. Any chance we can stop this trend? Once stopped slowely > clean up the past. > > e.g > ikev2_message.h 25 struct ike_sa; this is normal C > state.c 539 struct ike_sa *ike_sa(struct state *st) I don't know, the convention object name == constructor/cast name is very common in OO and any alternative would be longer :-( _______________________________________________ Swan-dev mailing list [email protected] https://lists.libreswan.org/mailman/listinfo/swan-dev
