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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

Jan



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