On Mon, Nov 27, 2023 at 12:23:38PM -0600, Rishabh Thukral wrote:
> 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
The same problem are --output <columns> we use in many tools. The
solution is hardcoded limit. It's usually 2 times number of the all
possible items.
> 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?
I think add
struct dmesg_control {
...
int timefmts[2 * __DMESG_NTIMEFMTS];
size_t ntimefmts;
}
would be enough. In the getopt_long() loop you can compare ntimefmts
with ARRAY_SIZE(ctl->timefmts) to check for the hardcoded limit (or
introduce a new function add_timefmt() to hide the detail).
> 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)`.
Well, option like --ctime is alias to --time-format ctime, so if we
will support multiple --time-format, then
dmesg --ctime --time-format delta
is the same as
dmesg --time-format ctime --time-format delta
It means two add_timefmt() calls in both cases.
And if there is a conflict, then --time-format should be the winner,
or don't support the interaction.
It's fine to define conflict between command line options. See
ul_excl_t excl[], there is already defined that you cannot use --raw
with something else.
> 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.
Sounds too complicated. It's better to be strict and easy to explain :-)
> Let me know your thoughts on this and I can proceed to implement changes
> based on your suggestions.
Keep it simple and stupid ;-)
Karel
--
Karel Zak <kzak@xxxxxxxxxx>
http://karelzak.blogspot.com
