[Swan-dev] Fwd: [Swan-commit] Changes to ref refs/heads/master

Andrew Cagney andrew.cagney at gmail.com
Wed Nov 21 21:28:02 UTC 2018


On Wed, 21 Nov 2018 at 12:45, Antony Antony <antony at phenome.org> 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 :-(


More information about the Swan-dev mailing list