Re: [Xen-devel] [PATCH] console: avoid printing no or null time stamps

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 07/02/2018 02:47 PM, Julien Grall wrote:
> On 06/26/2018 10:03 AM, Jan Beulich wrote:
> > > > > On 26.06.18 at 10:43, <julien.grall@xxxxxxx> wrote:
> > > On 26/06/18 08:24, Jan Beulich wrote:
> > > > During early boot timestamps aren't very useful, as they're all zero
> > > > (in "boot" mode) or absent altogether (in "date" and "datems" modes).
> > > > Log "boot" format timestamps when the date formats aren't available 
> > > > yet,
> > > > and log raw timestamps when boot ones are still all zero. Also add a
> > > > "raw" mode.
> > > >
> > > > Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> > > > ---
> > > > ARM side build-tested only; I'm in particular unsure whether
> > > > READ_SYSREG64(CNTPCT_EL0) can indeed be used right at start of day.
> > > On most of the platforms, the timer will have been correctly enabled by
> > > the firmware. However, on a few platforms it may require additional
> > > setup that will be performed by platform_init_time().
> > >
> > > In any case, CNTCPT_EL0 can always be read but may return garbage on
> > > those few platforms. I would not worry too much for those platforms
> > > thoughts.
> > Can this be detected, such that the function could be made return zero
> > instead until the necessary setup has happened?
> Not that early in the code. But as I said I would not worry too much.
>
> > > > @@ -698,26 +701,30 @@ static void printk_start_of_line(const c
> > > >        case TSM_DATE_MS:
> > > >            tm = wallclock_time(&nsec);
> > > > -        if ( tm.tm_mday == 0 )
> > > > -            return;
> > > > -
> > > > -        if ( opt_con_timestamp_mode == TSM_DATE )
> > > > -            snprintf(tstr, sizeof(tstr), "[%04u-%02u-%02u 
> > > > %02u:%02u:%02u] ",
> > > > -                     1900 + tm.tm_year, tm.tm_mon + 1, tm.tm_mday,
> > > > -                     tm.tm_hour, tm.tm_min, tm.tm_sec);
> > > > -        else
> > > > +        if ( tm.tm_mday )
> > > > +        {
> > > >                snprintf(tstr, sizeof(tstr),
> > > > -                     "[%04u-%02u-%02u %02u:%02u:%02u.%03"PRIu64"] ",
> > > > +                     opt_con_timestamp_mode == TSM_DATE
> > > > +                     ? "[%04u-%02u-%02u %02u:%02u:%02u] "
> > > > +                     : "[%04u-%02u-%02u %02u:%02u:%02u.%03"PRIu64"] ",
> > > >                         1900 + tm.tm_year, tm.tm_mon + 1, tm.tm_mday,
> > > >                         tm.tm_hour, tm.tm_min, tm.tm_sec, nsec / 
> > > > 1000000);
> > > I find this change rather difficult to read because the number of
> > > arguments for the 2 formats are different. It would be better to keep
> > > the two snprintf separately.
> > And I find the redundancy rather ugly to maintain, so I'd prefer to 
> > stick to
> > single invocation.
> Maybe it is for you. Not for me.
To give an example of my concern. The compiler (recent GGC) is happy the 
below snippet. This is just a potential for more programming error that 
are left even after review.
#include <stdio.h>

void main(void)
{
    printf((0) ? "Foo %s" : "Bar %d", 1);
}


Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel