On Sat, 22 Feb 2020 at 13:52, D. Hugh Redelmeier <[email protected]> wrote: > > | From: Andrew Cagney <[email protected]> > > | On Thu, 20 Feb 2020 at 17:00, D. Hugh Redelmeier <[email protected]> wrote: > | > > | > | From: Andrew Cagney <[email protected]> > | > > | > | On Thu, 20 Feb 2020 at 15:59, D. Hugh Redelmeier <[email protected]> > wrote: > | > | > > | > | > | From: Andrew Cagney <[email protected]> > | > | > | > | > 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 <[email protected]> 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 > [email protected] > https://lists.libreswan.org/mailman/listinfo/swan-dev _______________________________________________ Swan-dev mailing list [email protected] https://lists.libreswan.org/mailman/listinfo/swan-dev
