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

Thu Nov 22 21:15:43 UTC 2018

I dislike subject lines like this:
	Subject: [Swan-dev] Fwd: [Swan-commit] Changes to ref refs/heads/master

1. This is not in the swan-commit list as is implied by
   "[Swan-commit]"
   The "Fwd:" kind of covers that but not very effectively.

2. "Changes to ref refs/heads/master" is not at all helpful.
   I'd dig something out of the commit message itself.
	Subject: Re: demux: rename v2_msg_role() to msg_role(), move to demux.[hc]

| From: Andrew Cagney <andrew.cagney at gmail.com>

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

I don't know enough to have an opinion on this.

Eliminating includes is good.

| - 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 kind of guess that if v1 didn't have it, it didn't need it.

I would like each name that is in a file shared by v1 and v2 but that
only applies in one of those, to have a name with the appropriate v1_
or v2_ prefix.

I know prefixes make for long names.  That's why I think that the
concise v1_ and v2_ prefixes are much superior to ikev1_ and ikev2.

Within a version-specific function, there is no need for locals to have
a prefix.

Within a version-specific struct, there is no need for members to have
a prefix.

Within a version-specific enum, there IS a need for members to have a
prefix because uses of an enum member generally don't make the enum tag
manifest.

| From: Antony Antony <antony at phenome.org>

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

We don't have any other v1 or v2 things that I remember.  And if we
do, it's really minor.  The longer names would be a pointless burden.

I recollect a consensus on this in our Toronto meetings.  But perhaps
it was just a discussion that didn't reach closure.

v1_ and v2_, as prefixes.  Not in the middle or the end.

| From: Andrew Cagney <andrew.cagney at gmail.com>
| 
| On Wed, 21 Nov 2018 at 12:45, Antony Antony <antony at phenome.org> wrote:

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

Yes, that's what the noisiest advocate at our meeting (me) supported.

| > 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 :-(

I see the merit of both sides.

If you look at struct state, you will see that each member has an st_
prefix.  That's redundant but it makes grepping a lot more useful.
Well worth it.

(Try grepping for "\<next\>".  It is useless because there are so many
different members called "next".  There are 1280 lines in the source
tree that match that pattern.)

For the same reason, I tend not to like distinct meanings use the
same identifier.  But some are not much of a problem.

In this case, if you use grep for ike_sa, I imagine you get things that
are logically related so it doesn't seem like a big problem.  But it
probably does confuse ctags.

Why not call this function "new_ike_sa".  That reads better in C, I
think.  Oh, it's not a constructor.  It's kind of a getter.  But not
really.  The name isn't actually misleading, in my opinion; calling
it an example of an OO convention is misleading.

I find the cluster of functions that include ike_sa() somewhat
suspicious.

For example, I imagine that NULL is rarely an argument that should be
accepted by them.  I'd passert it away and make sure any callers that
really could have NULL handle it themselves.

For example, pexpect should not be a stable part of any code.  Just
like ??? comments.  pexpect calls should be converted to passerts or
code that actually handles the case.

| From: Paul Wouters <paul at nohats.ca>

| On Wed, 21 Nov 2018, Andrew Cagney wrote:

| > I believe the, er, guidance we were given was to use the shorter v[12].
| 
| I think we did agree on that?

I did :-)

Really?  Every reference to the struct tag immediately follows
"struct".  Everything else is a reference to the function.

It is confusing to grep and perhaps ctags but it should not confuse
people.  This matters to me, but only from a convenience standpoint.

| From: Paul Wouters <paul at nohats.ca>

| On Wed, 21 Nov 2018, Andrew Cagney wrote:

| > 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 think that is better, but I'm happy to also hear from Hugh,

Your comment prompted this message.