[Swan-dev] why there were useless assignments

D. Hugh Redelmeier hugh at mimosa.com
Wed Dec 5 23:06:17 UTC 2018


| commit c1f6a2bf6041abb5df918fcfd3fa118bb7c761c7
| Author: Paul Wouters <pwouters at redhat.com>
| Date:  Tue Dec 4 12:34:57 2018 -0500
| 
|     libswan: lswlog_proposal_info() do not set variable sep when no longer used afterwards

| commit f0b2361d0e6b20e22c7b5fe77dfe8cf1555901bc
| Author: Paul Wouters <pwouters at redhat.com>
| Date:   Tue Dec 4 12:33:29 2018 -0500
| 
|     pluto: ikev2_decode_peer_id_and_certs_counted() don't set c when no longer used

| commit f48425918e69c15edd8d526cbe4b396373d010d3
| Author: Paul Wouters <pwouters at redhat.com>
| Date:   Tue Dec 4 12:30:51 2018 -0500
| 
|     pluto: calc_skeyseed_v2() don't set next_byte when it won't be used anymore

| commit ce10f9bbaa4f3e0c1926e1a87c75d336b922aa38
| Author: Paul Wouters <pwouters at redhat.com>
| Date:   Tue Dec 4 11:30:01 2018 -0500
| 
|     pluto: initiate_ondemand_body() fix unused setting of loggedit

In each of these cases, the abstraction / invariant required these 
assignments.

Recommendation: restore these assignments.

Fallback recommendation: in place of these assignments, put a warning
comment to the effect that the invariant is no longer maintained.

The code works fine without these assignments.  But if you add more
code to one of these routines, the broken abstraction may cause the
code fail.  Formally: each assignment was required to maintain an
invariant.

Here's the tradeoff:

With assignments

- lint-like programs complain about a useless assignment (that's why I
  previously added an explanatory comment to each of these assignments)

Without assignment

- code modified in the obvious way will fail


Invariants:

c1f6a2bf6041abb5df918fcfd3fa118bb7c761c7:

	"sep" indicates what string needs to precede the next chunk
	(if any) to be added to the log line.

c1f6a2bf6041abb5df918fcfd3fa118bb7c761c7

	next_byte is the index of the next byte of keying material to
	use.

ce10f9bbaa4f3e0c1926e1a87c75d336b922aa38

	loggedit ("logged it", not "log edit") indicates whether
	demandbuf has been logged.

If an invaraint is not obvious to the reader (I think that it is),
then a comment describing the invariant should be added at the
variable's definition.


More information about the Swan-dev mailing list