[Swan-dev] [Swan-commit] Changes to ref refs/heads/master

[Andrew Cagney]

On 19 January 2016 at 12:01, D. Hugh Redelmeier <hugh at mimosa.com> wrote:
> On Mon, 18 Jan 2016, Andrew Cagney wrote:
>
> | From: Andrew Cagney <andrew.cagney at gmail.com>
>
> | On 18 January 2016 at 18:17, D. Hugh Redelmeier <hugh at vault.libreswan.fi> wrote:
> | > New commits:
> | > commit f86565435a7c00f8419ced6631d131d35d18011e
> | > Author: D. Hugh Redelmeier <hugh at mimosa.com>
> | > Date:   Mon Jan 18 18:16:33 2016 -0500
> | >
> | >     pluto ikev2_spdb_struct.c: initialize structs the old way for old GCC
> |
> | Actually this commit contained two changes.
>
> True.  I can't stop myself fiddling.
>
> | Firstly, I like this:
> |
> | +static bool print_join(struct print *buf, int n)
> |         if (n < 0 || buf->pos + n > sizeof(buf->buf))
> |                 return FALSE;
> |         buf->pos += n;
> |         return TRUE;
> |  }
> |
> | but needs a further tweak (I was also going through it carefully today
> | as part of more logging).  It should be:
> |
> |   if (n < 0)
> |     return FALSE;
> |   if (buf->pos + n >= sizeof(buf->buf)) {
> |     buf->pos = sizeof(buf->buf);
> |     return FALSE;
> |   }
> |
> | as, otherwise it will keep rewriting the end of the buffer.
>
> Right.  But that's not a big deal:
> - the buffer is sized so that this is unlikely to happen

I'm testing a change that accumulates the ascii representation of the
entire remote SA payload into that buffer. If no proposals match it
will get logged as a single line.

So I suspect it is no longer true.

> - there isn't actually a problem if it does happen.
>
> |  I'm
> | tempted to also also rewrite the tail of the buffer to "...".
>
> And you did.
>
> I think that buf[pos] should always be the '\0' at the end of the
> string within.  I've adjusted the code to reflect this.  I also used
> strcpy to plant the "...".
>
>
> | However, for this:
> |
> | -       struct print buf = {0};
> | +       struct print buf;
> | +       print_init(&buf);
>
> I think that the revised version is significantly more efficient.  The
> original code zeroed the whole buf member (currently 1024 bytes) where
> only the first needs to be zeroed.  As well as the time to do the writes,
> the extra zeroing adds to cache pressure.

It also leaves much of the buffer undefined - that always scares me
more than a few saved cpu cycles.

Anyway, this is all mute as I'm going to move the structure to the
heap: tiny uClinux stack; and buffers on the stack just scare me.


>
> | and:
> |
> | -               *proposal = (struct ikev2_proposal) {0};
> | +               static struct ikev2_proposal zero_proposal;     /* naturally zero */
> |
> | as discussed here,
> | http://stackoverflow.com/questions/1538943/why-is-the-compiler-throwing-this-warning-missing-initializer-isnt-the-stru
> | the code's behaviour is very well defined (both for C90 and C99), it
> | is just that GCC throwing a needless hissy fit.  Anyway, if we're
> | really going to pacify errors like this then the standard way is to
> | just add a coma like this:
> |
> |   struct print buf = {0,};
>
> Good to know.
>
> | or this:
> |
> |                         /* blat best with a new value */
> |                         *best = (struct ikev2_proposal) {
> |                                 .propnum = remote_proposal.isap_protoid,
> |                                 .protoid = remote_proposal.isap_protoid,
> |                                 .remote_spi = remote_spi,
> |                         };
>
> This looks quite a bit better since it isn't just zero.
>
> | and this:
> |
> | static struct ikev2_proposal default_ikev2_ike_proposal[] = {
> |         {
> |                 .protoid = IKEv2_SEC_PROTO_IKE,
> |                 .transforms = {
> |                         [IKEv2_TRANS_TYPE_ENCR] = TR(ENCR_AES_GCM16_256), ...
> |
> | (the latter two, didn't attract warnings :-)
>
> And this looks better too, for the same reason.
>
> We should follow the C standard, not GNU extensions, when possible.

Yes, C99:
https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html
https://gcc.gnu.org/onlinedocs/gcc/Compound-Literals.html

> ================
>
> Other bikeshedding:

Yes, constantly having to re-work important patches because someone
keeps tinkering with the colours gets tedious.

> - "struct print" isn't the greatest name.  It doesn't matter so much
>   when it is limited to one source file but perhaps it is worth
>   making generally available within pluto.

I took the idea from the code that deals with printing enums.  It
could do with being abstracted and generalised.  The obvious missing
method is buf_printf.

However, we know we're stable with SA proposals we've better things to
worry about.

>   Using typedef raises the level of abstraction slightly and saves keystrokes.
>   The name should suggest what it actually is.
>   I suggest typedef struct {...} fmt_buf;
>   But many other names would be as good or better.
>
> - all struct print variables are auto (good).  Each is considerable size
>   for stack-constrained environments so care should be taken to
>   shrinkwrap their scope.  The one example where this hasn't happened
>   is highlighted by the imaginative variable name "buf0".
>
> - There is a fair bit of control structure to make sure that
>   routines leave promptly when a struct print is full.  Is this worth
>   the bother?  It should not be needed for correctness (unless there
>   is some otherwise non-terminated loop).  It saves computation only
>   in cases that should not happen (as far as I know).  Are their cases
>   where an opponent could drive us into excessive computation?
>   I imagine that simplifying the control flow would be a net win.
>
> - I like throwing "const" around generously when pointers are involved
>
> ================
>
> I've pushed a few of the changes that I think are no brainers.

Andrew