From: Karel Zak <kzak@redhat.com>
To: Rishabh Thukral <rthukral@arista.com>
Cc: util-linux@vger.kernel.org, colona@arista.com
Subject: Re: [PATCH] util-linux/sys-utils dmesg support for additional human readable timestamp
Date: Mon, 4 Dec 2023 13:02:28 +0100 [thread overview]
Message-ID: <20231204120128.c3iysmc6i47bdkla@ws.net.home> (raw)
In-Reply-To: <CAPfLGEGXhMd5=f59PnA4rBKS6jDFP6=pWmqUhZR4PW5Sioj+=w@mail.gmail.com>
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@redhat.com>
http://karelzak.blogspot.com
next prev parent reply other threads:[~2023-12-04 12:02 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-10 23:26 [PATCH] util-linux/sys-utils dmesg support for additional human readable timestamp Rishabh Thukral
2023-11-23 9:30 ` Karel Zak
2023-11-27 18:26 ` Rishabh Thukral
[not found] ` <CAPfLGEGXhMd5=f59PnA4rBKS6jDFP6=pWmqUhZR4PW5Sioj+=w@mail.gmail.com>
2023-12-04 12:02 ` Karel Zak [this message]
-- strict thread matches above, loose matches on Subject: below --
2023-12-05 19:53 Rishabh Thukral
2023-12-12 10:44 ` Karel Zak
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20231204120128.c3iysmc6i47bdkla@ws.net.home \
--to=kzak@redhat.com \
--cc=colona@arista.com \
--cc=rthukral@arista.com \
--cc=util-linux@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).