[PATCH v3 0/7] Refactor syslog implementation

[Paul Eggert]

Thanks for looking into this. I'm reviewing the patches all in one diff 
rather than one patch at a time, as that's more convenient for me:


> -#ifdef SYSLOG_NAMES
> +#if defined(SYSLOG_NAMES) && defined(__USE_MISC)

Need spaces before parens. Better yet, omit the parens. Please do this 
systematically in #if.


> +  enum
>      {
> +      timestamp_size = sizeof "MMM DD hh:mm:ss ",
> +      bufs_size = 1024
> +    };

As these enums are used only once it might be more readable to eliminate 
them and replace their uses with their definiens, e.g.,

    char timestamp[sizeof "MMM DD hh:mm:ss "];
    ...
    char bufs[1024];

since the later code uses "sizeof timestamp" and "sizeof bufs" anyway 
(as that's less error-prone).


> +  /* "%h %e %H:%M:%S "  */

Please prefer "%b" to "%h" here and elsewhere, as they're equivalent and 
"%b" is more mnemonic (it's short for "%B").


> +  /* We deviate from RFC3164 which states timestamp should be in localtime

Please use imperative instead of plural form: "Deviate from" instead of 
"We deviate from". None of the new comments should need to use "we" or 
"us" or "our" or "ours".


> +  bool buf_malloced = false;

This local var isn't needed. You can remove it, and replace its use with 
"buf != bufs", which is like what the old code did; this is a bit more 
efficient, I expect.


> +  bool has_ts = __gmtime64_r (&now, &now_tm) != NULL;

It'll be slightly more efficient to replace this with:

    struct tm *now_tmp = __gmtime64_r (&now, &now_tm);
    bool has_ts = now_tmp != NULL;

and replace the "&now_tm" with "now_tmp" in the next __strftime_l call.


> +  /* In the highly unlike case of gmtime_r failure (the clock being
> +     INT_MIN + 1900 or follow INT_MAX + 1900) we skip the hostname so the
> +     message is handl as valid PRI but without TIMESTAMP or invalid TIMESTAMP
> +     (which should force the relay to add the timestamp itself).  */

Some English fixups. "unlike" -> "unlikely". No need for "highly". "the 
clock being INT_MIN + 1900 or follow INT_MAX + 1900" -> "tm_year out of 
int range". "we skip" -> "skip". "handl" -> "handled".

I don't understand the bit about "without TIMESTAMP or invalid TIMESTAMP
(which should force the relay to add the timestamp itself)". Since we're 
already departing from RFC 3164, aren't we already generating an invalid 
TIMESTAMP? And if so, why can't we output our own representation of the 
out-of-range timestamp, e.g., '@67768037170140800' to represent a 
timestamp that is 67768037170140800 seconds after the Epoch?

Better yet, we could output the correct year by dividing the __time64_t 
value by 12622780800 (60 * 60 * 24 * the number of days in 400 Gregorian 
years), running __gmtime64_r on the remainder, and adding 400 times the 
quotient to the tm_year that __gmtime64_r gives us; this computation 
will always succeed and so we won't need to worry about __gmtime64_r 
failure. On platforms with leap seconds this approach would go very 
slightly wrong on timestamps millions of years in the future but those 
timestamps are wrong anyway (due to leap seconds we don't know about 
yet, plus we'll switch to some approach other than leap seconds by then 
anyway).


> +  pid != 0 ? "[" : "", pid, pid != 0 ? "]" : ""

Is GCC smart enough to optimize this to be branch-free? If not, you can 
hand-optimize it as follows:

    "[" + (pid == 0), pid, "]" + (pid == 0)

> +               buf[bufsize - 1] != '\n' ? "\n" : "");

Similarly, this can be "\n" + (buf[bufsize - 1] == '\n').


> +  if (l < sizeof (bufs))

Omit the unnecessary parentheses (for consistency with the other code).
Also, this comparison isn't safe on admittedly-theoretical platforms 
where size_t is narrower than int. So I suggest:

    if (0 <= l && l < sizeof bufs)

which is clearer and should be equally efficient.

+      if (l + vl < sizeof bufs)

l + vl could have signed integer overflow, leading to undefined 
behavior. Also, this doesn't work if vl == -1. Also, we have the same 
theoretical problem as before. So change this to "if (0 <= vl && vl < 
sizeof bufs - l)".


> +      FILE *f = __open_memstream (&buf, &bufsize);
> +      if (f != NULL)

I'm not seeing what the memstream buys you here, compared to a simple 
malloc. You can't generate anything longer than INT_MAX bytes, since 
fprintf won't let you. And you already know how many bytes to allocate, 
from the returned value of the call to snprintf on the too-small stack 
buffer. So just call malloc and then call snprintf again; there's no 
need for a memstream. (The existing code already has this problem of 
course.)