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

D. Hugh Redelmeier hugh at mimosa.com
Sat Jan 5 02:57:40 UTC 2019


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

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

	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.

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

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

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

| >     - new names specify what should be included in the payload
| >       "sa": an SA (i.e. protoid and SPI)
| >       "chunk": a chunk
| >       "pl": a sub-payload will be added by the caller.

See organizing priciples (above).

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

Without renameing emit_v2N* to out_v2N*, some function names would be
recycled with new semantics.  So old callers would be wrong.  To
ensure that this could not happen, the emit_v2N* names were retired
(at least for now).

| >     - provide for callers to emit sub-payloads after the call.
| >       This will be exploited later.
| 
| Er, it's used above?

Yes, a little.  But there are several callers that should be reworked.

| >     - eliminated a leak in add_redirect_payload
| 
| That sounds serious enough to deserve a separate commit.

Sure.

My git skills make this a bit awkward.

I frequently find these and fix them casually, just like spelling 
mistakes.

The most frequent cause of these is return statements.  Often error 
returns.


More information about the Swan-dev mailing list