Hi Rishabh, sorry for delay On Fri, Nov 10, 2023 at 03:26:12PM -0800, Rishabh Thukral wrote: > This change updates the --time-format option in dmesg to enable the > user to specify it multiple times with different formats with each good idea > input providing a timestamp format among the currently supported > choices. This enables seeing the dmesg logs in both seconds since boot > and human readable format simultaneously in each row of the log output. > The sequence of timestamp format is fixed and independent of the order > in which the user provides the desired formats. and would be better to support arbitrary (user defined) order? ctl->time_fmts[ ctl->ntime_fmts++ ] = DMESG_TIMEFTM_FOO; ctl->time_fmts[ ctl->ntime_fmts++ ] = DMESG_TIMEFTM_BAR; and print it by a simple loop for (i = 0; i < ctl->ntime_fmts; i++) switch (ctl->time_fmts[ i ]) { ... } I guess the implementation will be more simple. > - if (ctl->time_fmt != DMESG_TIMEFTM_RELTIME) { > + if (ctl->time_fmts[DMESG_TIMEFTM_RELTIME] == TIMESTAMP_FMT_UNSET) { This code pattern is used pretty often in your patch, maybe add macros #define is_timefmt_set(c, f) ((c)->time_fmts[DMESG_TIMEFTM_ ##f] == TIMESTAMP_FMT_SET)) #define is_timefmt_unset(c, f) ((c)->time_fmts[DMESG_TIMEFTM_ ##f] == TIMESTAMP_FMT_UNSET)) #define is_timefmt_initset(c, f) ((c)->time_fmts[DMESG_TIMEFTM_ ##f] == TIMESTAMP_FMT_INIT_SET)) and use if (is_timefmt_set(ctl, RELTIME)) in code. > @@ -1388,9 +1416,11 @@ int main(int argc, char *argv[]) > .action = SYSLOG_ACTION_READ_ALL, > .method = DMESG_METHOD_KMSG, > .kmsg = -1, > - .time_fmt = DMESG_TIMEFTM_TIME, I guess you can use .time_fmts[DMESG_TIMEFTM_TIME] = TIMESTAMP_FMT_INIT_SET, > .indent = 0, > }; > + memset(ctl.time_fmts, 0, > + TOTAL_DMESG_TIMESTAMP_FORMATS_SUPPORTED * sizeof(*(ctl.time_fmts))); You do not need memset() here. All unspecified struct fields in 'ctl' are set zero according to C standards (if any other fields are explicitly initialized). > + ctl.time_fmts[DMESG_TIMEFTM_TIME] = TIMESTAMP_FMT_INIT_SET; > int colormode = UL_COLORMODE_UNDEF; > enum { > OPT_TIME_FORMAT = CHAR_MAX + 1, > @@ -1475,7 +1505,9 @@ int main(int argc, char *argv[]) > ctl.action = SYSLOG_ACTION_CONSOLE_ON; > break; > case 'e': > - ctl.time_fmt = DMESG_TIMEFTM_RELTIME; > + if (ctl.time_fmts[DMESG_TIMEFTM_TIME] == TIMESTAMP_FMT_INIT_SET) > + ctl.time_fmts[DMESG_TIMEFTM_TIME] = TIMESTAMP_FMT_UNSET; > + ctl.time_fmts[DMESG_TIMEFTM_RELTIME] = TIMESTAMP_FMT_SET; This complicated setup will be unnecessary if we will not need fixed order, right? :-) Karel -- Karel Zak <kzak@xxxxxxxxxx> http://karelzak.blogspot.com