[Swan-dev] pluto: rename and remodularize emit_v2N*

[Andrew Cagney]

On Fri, 4 Jan 2019 at 21:57, D. Hugh Redelmeier <hugh at mimosa.com> wrote:
>
> | From: Andrew Cagney <andrew.cagney at gmail.com>
> | Date: Thu, 3 Jan 2019 00:03:48 -0500
>
> sorry for the slow response.
>
>
> | On Wed, 2 Jan 2019 at 15:55, D. Hugh Redelmeier <hugh at vault.libreswan.fi> wrote:| >
> | > commit 20f4002247ba4540cdc3b4ebe6f7c73828682649
> | > Author: D. Hugh Redelmeier <hugh at mimosa.com>
> | > Date:   Wed Jan 2 15:54:08 2019 -0500
> | >
> | >     pluto: rename and remodularize emit_v2N*
> |
> | Why?   We had emit_v2V() emit_v2N() emit_v2UNKNOWN() et.al., now we don't?
> Several reasons.
>
> - I found the names confusing so I came up with an organizing
>   principle:

What's confusing?  Two things were changed: emit_*() prefix and the
*v2N<suffix>().

For the prefix rename, the rationale is:

> | >     - changed emit_* to out_* so uncommited change won't silently fail
> |
> | I've no clue what this even means?
>
> By "uncommited change", I mean code that has not yet been committed to
> HEAD.  It might have been written to call the old functions.

I'm struggling to buy this.  Changing the suffixes and signatures
already broke the code?

Before I could look for emit_v2*() and failing that *_emit_*() and
likely find the function needed to emit an IKEv2 payload.  Now I
can't.  Instead, some are called out_*() - confusing them with the
primitive out*() routines in packet.[hc] - and some are called
emit_*().

Should we restore the above emit_v2() naming schema?

For the suffix, first a question.  For the names:

bool ship_v2Nsp(enum next_payload_types_ikev2 np,
           v2_notification_t type,
           const chunk_t *n_data,
           pb_stream *rbody);
bool ship_v2Ns(enum next_payload_types_ikev2 np,
           v2_notification_t type,
          pb_stream *rbody);

what exactly do <sp> and <s> stand for?  I could never figure it out -
there was no breadcrumb.  So when fixing the CHILD SA SPI code, and
eliminating ship_() (it sounds like what's what was generated as set
sail) I applied and documented the "organizing principle":

* Wrappers for common cases:
*
* emit_v2Ntd: emit_v2N(..., T[YPE], D[ATA], ...)
* emit_v2Nt(): emit_v2N(..., T[YPE], ...)
*
* where remaining fields are defaulted to:
*
* - C+RESERVED: ISAKMP_PAYLOAD_NONCRITICAL
* - Protocol ID: IKEv2_SEC_PROTO_NONE
* - SPI: none
* - Notification Data: empty
*
* These cases are common since

While it might seem confusing, it is at least documented.  Now we've
got the new organizing principle:

-/* add notify payload to the rbody */
+/*
+ * ship_v2N: add notify payload to the rbody
+ * (See also specialized versions ship_v2Nsp and ship_v2Ns.)

er?

>         The name would specify what components of the notification the
>         caller would supply.  The rest would be defaulted.

This is ironic, at the very beginning there was only ship_v2N() that
handled everything.  But then the wrappers ship_v2sp() and ship_v2s()
were added.

("chunk" isn't even a component of a notification payload,
Notification Data is).

>         Each call would include the notification type, so that could
>         be implicit in the name out_v2N.
>
> - It was convenient to allow some of the parameters to be NULL.
>   This cut down on the number of variants: in some cases, variants
>   of out_v2N* routines and sometimes variants of their callers.
>   I tried to put /* optional */ on those parameters in the function
>   prototypes.

3 vs 3?

> - I tried to structure the routines to avoid code duplication.
>
> - In some cases, the "chunk *ndata" way of specifying the subpayload
>   of a Notification isn't the best so there are out_v2N* routines
>   that allow the caller to emit subpayloads via the normal packet.h
>   routines.
>
>   Why is this better?  Because packet.h already handles
>   host-to-network byte ordering, logging of payloads, and packet
>   overflow.  It also avoids heap usage (error prone and expensive).

no one is debating this, as I noted:

> | My best guess is that it was motivated by the comment and code:
> |
> | /*
> | * XXX: can this share code with emit_v2N() above?
> | */
>
> That's one thing that was improved.

the solution you came up with was to add out_v2N(,
pb_stream*nested_pbs).  That is a every small and very smart change.
And one that doesn't need to re-arrange the deck chairs.

> | These assertions were removed.  Why?
> |
> | passert((protoid == PROTO_v2_RESERVED) == (spi == NULL));
> | passert((protoid == PROTO_v2_AH || protoid == PROTO_v2_ESP) == (spi != NULL));
>
> Those cases are caught and reported by dbg calls (not my preference).

I'm not so sure, the above checked very specific requirements of the
RFC, was regardless of the Notification Type, and exploded when it
failed.  So I still don't understand why they were removed.

As for the replacement:

        if (protoid == PROTO_v2_RESERVED || spi == NULL) {
            dbg("XXX: type requires SA; missing");
        }

the dbg() message is bogus.  BTW, do those errors even appear in the
test results?

Should we put the passert()s back?

Andrew