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:)
_______________________________________________
Swan-dev mailing list
Swan-dev@lists.libreswan.org
https://lists.libreswan.org/mailman/listinfo/swan-dev
