[Swan-dev] xauth_send_request has a comment that confuses me

[Antony Antony]

On Mon, Oct 02, 2017 at 12:43:51PM -0400, Paul Wouters wrote:
> On Sun, 1 Oct 2017, D. Hugh Redelmeier wrote:
> 
> > At the end of xauth_send_request:
> > 
> >        /* RETRANSMIT if Main, SA_REPLACE if Aggressive */
> > 	/* ??? the actual code seems to force EVENT_v1_RETRANSMIT */
> > 	if (st->st_event->ev_type != EVENT_v1_RETRANSMIT) {
> > 		delete_event(st);
> >                event_schedule_ms(EVENT_v1_RETRANSMIT,
> >                  		st->st_connection->r_interval, st);
> >        }
> > 
> > I don't understand how the comment matches the code.  Is the comment
> > correct?  Is the code?  If both are correct, I suggest the comment
> > needs expansion to make this clear.
> 
> The comments go back to the initial import of openswan 2.0.0.
> 
> I don't understand the comment and I'm happy the code doesn't follow it.
> 
> I've removed the comments.

well if the comment was true I could avoid double sending in server.c
resend_ike_v1_msg see the comment 0cab34e4e
if (st->st_state == STATE_XAUTH_R0) {
	       /* may cause double in aggrmode=yes, it is not fatal, fixme 
		event_schedule_ms(EVENT_v1_SEND_XAUTH, EVENT_v1_SEND_XAUTH_DELAY, st);
}

Paul, do you know how to check in resend_ike_v1_msg wheater a st is Main 
mode or Aggressive mode? Both cases current state is STATE_XAUTH_R0. I want 
to know the previous one, STATE_AGGR_R2 or STATE_MAIN_R3. 

> > Bonus issue:
> > 
> > - The same comment and code appear in modecfg_send_request.
> > 
> > - The same comment and almost the same code appear in
> >  modecfg_send_set.
> > 
> > Should this be turned into a function?
> 
> It's 3 lines, so I'm not worried. But I don't mind either.
> 
> > Is the difference in the code in modecfg_send_set important?
> 
> Are talking about the order of record_and_send_ike_msg() and
> the event rescheduling? Or the fact that it does not check
> the existing event and just always replaces it?
> 
> The first cause has a comment clarifying it:
> 
>         /* Schedule retransmit before sending, to avoid race with master thread */
> 
> I don't understand the comment, because I'm pretty sure this is the main thread?
> The comment comes from openswan 2.1.4 when xauth was added.
> 
> Maybe Antony can tell why we check for EVENT_v1_RETRANSMIT and
> EVENT_NULL before setting it to EVENT_v1_RETRANSMIT? My guess is
> that checking for EVENT_v1_RETRANSMIT just ensures the old timer
> remains, but I'm unsure why. I also don't understand why not
> having an event would be reason to still not schedule one?

my guess, this is happening in a thread. And the main thread event may occur 
while xauth is working.

Currently, I would suggest delete_event(st) at the begining. May be for 
another day to look at, I had enough fun with ikev1 xauth in the past couple 
of weeks:)