[Swan-dev] loop structure of score_ends_seclabel()

[D. Hugh Redelmeier]

[These comments would have been better made during code review.  I'm
sorry that I didn't have time to do this earlier.  Idiosyncratic
excuse: I gain understanding while nit-picking (see
a5c5d5ca7828f3b3aa0384b22cc9154acd23f985)]

The loop structure of score_ends_seclabel doesn't look right to me.
But I don't actually know the logic that it is supposed to implement.
I may also be misunderstanding the code.

for each tsi
	if there's a match with the spd "this"
		remember the match_i index

		for each tsr
			if there is a match with the spd "this"
				remember the match-r index

What's suspicious:

- There is no early-out for either loop for a match, so the last match 
  will be remembered.  Not the best match.  If any match will do, surely
  we should stop the loop at the first.

- Both loops are matching against spd "this".  Surely "that" should be 
  involved in one or the other

- the variables changed by the outer loop do not affect the inner
  loop.  It makes no sense to nest them.  My guess: there is a reason
  in the specs but it isn't in the code.

- match_r is initialized to false before the outer loop.  Perhaps it
  should also be initialized before the inner loop.  This depends on
  there being a reason to nest the loops.

- consider the code:
			if (cur_i->sec_label.len == 0) {
			        /* ??? complain loudly */
		        	continue;
                        }
  (There is a related test in the inner loop too.)

  I seem to remember being told that the labels, even though they are
  a sequence of bytes with a length, are also NUL-terminated strings.
  If so
	1. this is dumb design: it is a redundant representation and
	   pointlessly introduces the ability to have invalid representations
	   (the NUL might be missing.)

	2. the test, if it is meant to check for this problem, is only
	   partial.  Something like this should be used.

			if (cur_i->sec_label.len == 0 ||
			    cur_i->sec_label.ptr[cur_i->sec_label.len-1] != '\0')
			{
			        /* ??? complain loudly */
		        	continue;
                        }
	3. but better: the test should be done when the data-structure is ingested,
	   as part of input validation.  Then the rest of Pluto need not deal with
	   this.

        4. this requirement should be documented in the RFC and in the code.
	   (Perhaps it is.)