[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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





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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.