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

[Julien Grall <julien.grall@xxxxxxx>]

On 03/07/18 07:29, Jan Beulich wrote:
> > > > On 02.07.18 at 22:28, <cardoe@xxxxxxxxxx> wrote:
> > On Mon, Jul 02, 2018 at 02:47:42PM +0100, 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:
> > > > > > @@ -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.
> > I'm in agreement with Julien. Its not easy to follow and certainly not
> > having the number of arguments line up looks like a "code smell" to me.
> Having looked again, do both of you realize that I didn't do this change
> just because I like it better this way, but first and foremost to avoid
> another level of indentation (plus to make more obvious the similarity
> between the two format strings)? The alternative of
>
>          if ( tm.tm_mday == 0 )
>              /* nothing */;
>          else if ( opt_con_timestamp_mode == TSM_DATE )
>          {
>              ...
>
> (to also avoid the extra level) doesn't look better to me.
Yes, I realized that when writing the e-mail yesterday. The extra level 
would not looked so bad compare to potential format mismatch not 
detected by the compiler.
Cheers,

--
Julien Grall

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