Hi Karel,
Thanks for reviewing the patch and sharing your feedback. I'll work on
the changes that are recommended. I also wanted to share the thought
behind this design approach to stick with a fixed order of timestamp
formats. Since we are allowing users to specify the `--time-format`
option multiple times, there is no potential limit to how many times
the user might specify it. There could be duplicate entries of the
same time format appearing again and again and we'll have to collect
the entire user input requiring an arbitrarily large input buffer.
With a fixed-size buffer, there might be a possibility of buffer
overflow. One alternative here is to have a complex data structure
that is fixed in size and keeps track of unique time formats in a
sequence of appearance in user input. Would it be a good idea to have
something like this?
The existing approach of marking which timestamps were included in
user input makes it easier to deal with the interactions with other
time-format options like `-d (--delta)`, `-T (--ctime)`, `-H
(--human)`, `-e (--reltime)`, `-r (--raw)`, `-t (--notime)`. If we
maintain a list of all user input we might have to insert, delete, or
replace some entries based on these other specified options and follow
a convention in what order should new values be inserted.
I added the value TIMESTAMP_FMT_INIT_SET in addition to just SET and
UNSET counterparts to support the overriding behavior that we
currently have where the default value is overridden if the user has
specified `--time-format` option but it should not be overridden if
the user has explicitly specified the default option as one of the
timestamp formats. You're right, we would not need this if we just
maintained a list of user input.
Let me know your thoughts on this and I can proceed to implement
changes based on your suggestions.
Regards
Rishabh Thukral
On Thu, Nov 23, 2023 at 3:30 AM Karel Zak <kzak@xxxxxxxxxx> wrote:
>
>
> 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
>
