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

[D. Hugh Redelmeier]

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

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

================

Other bikeshedding:

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

  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.