Re: [PATCH] Prepend timestamp in msgbuf
On Mon, Oct 17, 2011 at 5:18 PM, Arnaud Lacombe wrote: > Hi, > > On Mon, Oct 17, 2011 at 6:22 PM, Ed Schouten wrote: >> Ah, missed something. >> >>> + getnanouptime(&ts); >>> + err = snprintf(buf, sizeof buf, "[%zd.%.6ld] ", >>> + ts.tv_sec, ts.tv_nsec / 1000); >> >> It seems we also have a getmicrouptime(), which returns a struct >> timeval. > fixed. > >> Also a more general question: is it actually safe to call >> getnanouptime() here? This code gets executed from an arbitrary context, >> right? >> > right, but getmicrouptime() is not doing much magic. Just reading a > cached value, do an arithmetic conversion. I do not really see any > unsafe part. Based on glancing around other areas of the kernel, I'd assume that using this KPI as-is is fine because I don't see any locking employed elsewhere... -Garrett ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: [PATCH] Prepend timestamp in msgbuf
Hi, On Mon, Oct 17, 2011 at 6:22 PM, Ed Schouten wrote: > Ah, missed something. > >> + getnanouptime(&ts); >> + err = snprintf(buf, sizeof buf, "[%zd.%.6ld] ", >> + ts.tv_sec, ts.tv_nsec / 1000); > > It seems we also have a getmicrouptime(), which returns a struct > timeval. fixed. > Also a more general question: is it actually safe to call > getnanouptime() here? This code gets executed from an arbitrary context, > right? > right, but getmicrouptime() is not doing much magic. Just reading a cached value, do an arithmetic conversion. I do not really see any unsafe part. - Arnaud > -- > Ed Schouten > WWW: http://80386.nl/ > ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: [PATCH] Prepend timestamp in msgbuf
Hi,
On Mon, Oct 17, 2011 at 6:19 PM, Ed Schouten wrote:
> Hi Arnaud!
>
> * Arnaud Lacombe , 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
> WWW: http://80386.nl/
>
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: [PATCH] Prepend timestamp in msgbuf
Ah, missed something. > + getnanouptime(&ts); > + err = snprintf(buf, sizeof buf, "[%zd.%.6ld] ", > + ts.tv_sec, ts.tv_nsec / 1000); It seems we also have a getmicrouptime(), which returns a struct timeval. Also a more general question: is it actually safe to call getnanouptime() here? This code gets executed from an arbitrary context, right? -- Ed Schouten WWW: http://80386.nl/ pgpOeBagOuQDB.pgp Description: PGP signature
Re: [PATCH] Prepend timestamp in msgbuf
Hi Arnaud!
* Arnaud Lacombe , 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? Also. please use %jd and cast ts.tv_sec to intmax_t. The size of
time_t and size_t are independent. 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.
> + 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);
> - intmsg_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. :-)
--
Ed Schouten
WWW: http://80386.nl/
pgpFwZRV69K8d.pgp
Description: PGP signature
