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

[Andrew Cagney]

On Sat, 22 Feb 2020 at 13:52, D. Hugh Redelmeier <hugh at mimosa.com> wrote:
>
> | 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.

Here's another:

+       struct fd *const_stupidity;
+       /* global_whackfd */
+       const_stupidity = (struct fd*) (*logp)->global_whackfd;
+       (*logp)->global_whackfd = NULL;
+       close_any(&const_stupidity);
+       /* object_whackfd */
+       const_stupidity = (struct fd*) (*logp)->object_whackfd;
+       (*logp)->object_whackfd = NULL;
+       close_any(&const_stupidity);

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

So why did it hang ...

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

If you want to figure out why the old code didn't work, start here.

commit 9dc14180feb9669766686bc4b40c860eeab885a7
Author: Andrew Cagney <cagney at gnu.org>
Date:   Sun Nov 3 08:20:52 2019 -0500

    logging: only dup a whackfd when needed, typically when storing it
in a heap object

    For instance, when copying the "global" whackfd into pending; and
    copying pending's whackfd into the state.  Conversely, don't dup the
    wackfd when it is being passed as a parameter on the stack.

    In first_pending(), explicitly release the child's old whackfd before
    storing a duped new value.

    In release_pending_whacks(), when ST is a CHILD SA and it's IKE SA has
    a matching whackfd, release all the IKE SA's pending children (stops
    whack being left haing when the child fails; the code was trying to
    release the CHILD SA's pending children ...).

    In flush_incomplete_child(), explictly release the child (stops extra
    log messages sneaking through to whack).

    This replaces "ownership transfered".

Note that it was pushed _before_ 'struct fd' - I was using
experimental struct fd code, and its reference count logging, to debug
the dup(fd).  New code pulling bugs out of old code is a sure sign
that it is time to switch.

> [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?

Me? No.

The old code, presumably assuming things would succeed, was duping the
fd into a parameter:

-               add_pending(dup_any(whack_sock), ike, c, policy, 1,
-                       predecessor == NULL ?
-                         SOS_NOBODY : predecessor->st_serialno,
-                       uctx);
+               add_pending(whack_sock, ike, c, policy, 1,
+                           predecessor == NULL ?
+                           SOS_NOBODY : predecessor->st_serialno,
+                           uctx);

-       initiate_connection(c->name, dup_any(st->st_whack_sock),
-                               empty_lmod, empty_lmod,
-                               NULL);
+       initiate_connection(c->name, st->st_whack_sock,
+                           empty_lmod, empty_lmod,
+                           NULL);

which meant, when things went wrong the callee, had to undo the
damage.  I call that digging a hole.

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