Re: [PATCH] Prepend timestamp in msgbuf

From: Arnaud Lacombe <lacombar_at_gmail.com>
Date: Mon, 17 Oct 2011 20:16:07 -0400
Hi,

On Mon, Oct 17, 2011 at 6:19 PM, Ed Schouten <ed_at_80386.nl> wrote:
> Hi Arnaud!
>
> * Arnaud Lacombe <lacombar_at_gmail.com>, 20111017 22:41:
>> +             buf[0] = '\0';
>> +             getnanouptime(&ts);
>> +             err = snprintf(buf, sizeof buf, "[%zd.%.6ld] ",
>> +                 ts.tv_sec, ts.tv_nsec / 1000);
>
> What's the use of buf[0] = '\0'? snprintf() will overwrite it anyway,
> right?
leftover from previous debug I guess; fixed.

> Also. please use %jd and cast ts.tv_sec to intmax_t. The size of
> time_t and size_t are independent.
fixed.

> As far as I know, you should be able
> to use a 64-bit time_t on i386 by simply changing the typedef and
> recompiling everything.
>
As long as you do not care about breaking the ABI, yes. But yet, the
kernel and the userland may not need to each have the same
representation of what `time_t' is, as long as they agree on the
interface.

>> +             bufp = buf;
>> +             while (*bufp != '\0') {
>> +                     __msgbuf_do_addchar(mbp, seq, *bufp);
>> +                     bufp++;
>> +             }
>
> It would be nicer to write this as follows:
>
>                for (bufp = buf; *bufp != '\0'; bufp++)
>                        __msgbuf_do_addchar(mbp, seq, *bufp);
>
fixed.

>> -     int        msg_needsnl;         /* set when newline needed */
>> +     uint32_t   msg_flags;
>
> Why change this to uint32_t instead of leaving it the way it is (or
> changing it to unsigned int)? Even though they are likely to be equal in
> size, there is no reason why msg_flags must be 32 bits. :-)
>
made it `unsigned int'; I don't like playing with signed bit-field.

 - Arnaud

> --
>  Ed Schouten <ed_at_80386.nl>
>  WWW: http://80386.nl/
>
Received on Mon Oct 17 2011 - 22:16:09 UTC

This archive was generated by hypermail 2.4.0 : Wed May 19 2021 - 11:40:19 UTC