[Swan] awkward code in ikev1.c

[Paul Wouters]

On Thu, 28 Feb 2013, D. Hugh Redelmeier wrote:

> I was just reading programs/pluto/ikev1.c and came across some awkward
> code.  I don't feel safe just checking in these changes so I'll post them
> here.

I do think your way is much clearer, I'm okay with it.

> The original logic is somewhat contorted.  The switch can only go one
> way!  The complex if can be absorbed into the switch.  Perhaps there
> is an intent that isn't actually captured by the code.

I'm not seeing anything obvious for that...

> I think that we can simplify the access to the message ID to:
> 	md->hdr.isa_msgid
> That would eliminate the need for the last if.  If I'm wrong and
> st->msgid isn't the same as md->hdr.isa_msgid when st!=NULL, it would
> still be a good thing to use when st==NULL.
>
> I think that the st_msgid and isa_msgid fields are in network order.  To
> print them with %08x, you need to wrap them in ntohl().
>
> In the good old days (pre C99 standard), int could be 16 bits. So we
> used unsigned long as a safe container for 32-bit quantities.  Current
> C standards require int to be at least 32 bits.  The old code is safe.
> I'd prefer we still wrote the code that way.  (Why do you think ntohl
> ends in l?)
>
> ==== Original:
>
> 	    if(p->payload.notification.isan_type != R_U_THERE
> 	       && p->payload.notification.isan_type != R_U_THERE_ACK
> 		&& p->payload.notification.isan_type != ISAKMP_N_CISCO_LOAD_BALANCE
> 	       && p->payload.notification.isan_type != PAYLOAD_MALFORMED) {
>
> 		switch(p->payload.notification.isan_type) {
> 		case INVALID_MESSAGE_ID:
> 		default:
> 		    if (st!= NULL) {
> 		    	loglog(RC_LOG_SERIOUS
> 			   , "ignoring informational payload, type %s msgid=%08x"
> 			   , enum_show(&ipsec_notification_names
> 				       , p->payload.notification.isan_type), st->st_msgid);
> 		    }
> 		    else {
> 		    	loglog(RC_LOG_SERIOUS
> 			   , "ignoring informational payload, type %s on st==NULL (deleted?)"
> 			   , enum_show(&ipsec_notification_names
> 				       , p->payload.notification.isan_type));
>
> 		    }
> 		}

This crappy if statement looked familiar. git blame agrees. I did it:

commit 15e3c510c513b10371a9632aabce2ae5e1ccfcf5
Author: Paul Wouters <paul at xelerance.com>
Date:   Sat Oct 11 01:24:18 2008 -0400

     Work around for bug #994, where we seem to be trying to reference the
     state st while we don't have it, to log a warning.


> ==== Revised:
> 	    switch (p->payload.notification.isan_type) {
> 	    case R_U_THERE:
> 	    case R_U_THERE_ACK:
> 	    case ISAKMP_N_CISCO_LOAD_BALANCE:
> 	    case PAYLOAD_MALFORMED:
> 		/* silently ignore these payloads ??? */
> 		break;

Perhaps we should log these at some kind of debug level?
We are definately dealing with these cases, but obviously not here.

> 	    case INVALID_MESSAGE_ID:	/* why make this case explicit??? */
> 	    default:
> 		loglog(RC_LOG_SERIOUS
> 		    , "ignoring informational payload, type %s msgid=%08lx"
> 		    , enum_show(&ipsec_notification_names, p->payload.notification.isan_type)
> 		    , (long unsigned)ntohl(md->hdr.isa_msgid));
> 		}

I am not sure why we are doing this explicitely for INVALID_MESSAGE_ID.

Paul