[Swan-dev] pluto: sprinkle const on many struct fd * parameters

[D. Hugh Redelmeier]

| From: Andrew Cagney <andrew.cagney at gmail.com>

| On Thu, 20 Feb 2020 at 17:00, D. Hugh Redelmeier <hugh at mimosa.com> wrote:
| >
| > | From: Andrew Cagney <andrew.cagney at gmail.com>
| >
| > | On Thu, 20 Feb 2020 at 15:59, D. Hugh Redelmeier <hugh at mimosa.com> wrote:
| > | >
| > | > | From: Andrew Cagney <andrew.cagney at gmail.com>
| > |
| > | > If one declares all possible struct fd * things const, the absence of
| > | > const highlights where references could go wrong.
| > |
| > | Why?
| >
| > Those are the only places that can (directly or indirectly) change a
| > reference count.
| 
| I don't follow.
| 
| I attribute the bugs in the old code to it trying to be too clever by
| minimising the number of places that a reference needed to be taken.

How so?  I think that it had just the right number.

| This resulted in code never being sure if it needed to take a
| reference, or free the reference because it had encountered an error.

You are anthropomorphising.  Some programmers didn't understand the 
discipline (it was essentially like any non-auto resource allocation 
discipline).  Clearly this was not documented well enough (by me).

[I'm looking at freeswan-2.06 for the following.]

In Pluto, there are a bunch of global variables that conceptually have the 
scope of a single transaction (where transaction is roughly what is done 
per-event in the event loop).  The macro GLOBALS_ARE_RESET was meant
to discover some mistakes with these variables.  There is a cluster
intended for providing context for logging.

whack_log_fd was one of these per-transaction globals.  You've
replaced it with a bunch of parameters.

The rules for duping the FD were fairly simple.  If you wished to
preserve the fd past the current transaction, dup it (because that fd
would be closed at the end of the transaction).  When is it preserved?

- on initiation of a negotiation: the negotiation will span several
  transactions and each step needs to be logged.  As a useful
  side-effect, the whack command will not complete until that fd gets
  closed.  The whack fd will end up in the struct state.

- similar case: when a pending negotiation is recorded.  The whack fd
  will end up in the pending datastructure.

- analogous case: when replacing an SA.

- in a continuation for opportunistic or key_add

This is a lot like the discipline needed for heap string references.

| The new code takes a reference when ever the pointer is copied to/from
| the heap (or heap to heap).
| 
| If the object is made const and we make adding references harder we
| just encourage code that incorrectly copies the actual pointer.

These are harmless unless someone casts away the const.  That assumes
that const isn't used on a declaration that isn't auto (it generally
should not be).