From: Karel Zak <kzak@redhat.com>
To: Edward Chron <echron@arista.com>
Cc: util-linux@vger.kernel.org, colona@arista.com
Subject: Re: [PATCH] util-linux/sys-utils dmesg add PRINTK_CALLER id support
Date: Fri, 8 Dec 2023 12:42:10 +0100 [thread overview]
Message-ID: <20231208114210.h2rmm7g44tjhjhco@ws.net.home> (raw)
In-Reply-To: <20231206210449.27011-1-echron@arista.com>
On Wed, Dec 06, 2023 at 01:04:49PM -0800, Edward Chron wrote:
> sys-utils/dmesg.c | 108 +++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 107 insertions(+), 1 deletion(-)
Note for list-only followers; Edward will create a PR on GitHub
https://github.com/util-linux/util-linux/issues/2609
> +static const char PROC_SYS_KERN_PID_MAX[] = "/proc/sys/kernel/pid_max";
Please, add it as _PATH_PROC_PIDMAX macro to /include/pathnames.h
> +#define PID_CHARS_MAX 16
It's usually better to keep it based on some type, we usually use
something like:
#define PID_CHARS_MAX sizeof(stringify_value(LONG_MAX))
> +static size_t max_threads_id_size(void)
> +{
> + char taskmax[PID_CHARS_MAX];
char taskmax[PID_CHARS_MAX] = { '\0' };
> + ssize_t rdsize;
> + int fd;
> +
> + fd = open(PROC_SYS_KERN_PID_MAX, O_RDONLY);
> + if (fd == -1)
> + return (size_t)5;
> +
> + memset(taskmax, 0, sizeof(taskmax));
... and don't use memset() to initialize.
> + rdsize = read(fd, taskmax, sizeof(taskmax));
> + if (rdsize == -1)
> + return (size_t)5;
Maybe we can avoid hardcoded numbers in code
#define PID_CHARS_DEFAULT sizeof(stringify_value(SHORT_MAX))
> +static const char *parse_callerid(const char *p_str, const char *end,
> + struct dmesg_record *p_drec)
> +{
> + static const char cid[] = "caller=";
Use macro, compiler will save the string to the right place.
#define DMESG_CALLER_PREFIX "caller="
#define DMESG_CALLER_PREFIXSZ (sizeof(DMESG_CALLER_PREFIX) - 1)
> + const char *p_after;
> + const char *p_next;
> + size_t cid_size;
> + char *p_cid;
> +
> + p_cid = strstr(p_str, cid);
> + if (p_cid != NULL && p_drec != NULL) {
> + p_next = p_cid + sizeof(cid)-1;
> + p_after = skip_item(p_next, end, ",;");
> + cid_size = p_after - p_next - 1;
You should verify that cid_size < sizeof(p_drec->caller_id) before you
call strncpy().
> + memset(p_drec->caller_id, 0, sizeof(p_drec->caller_id));
> + strncpy(p_drec->caller_id, p_next, cid_size);
Please, xstrncpy() (from include/strdutils.h) to be sure it's zero terminated.
> + return p_after;
> + }
> + return p_str;
> +}
> +
> /*
> * Parses one record from syslog(2) buffer
> */
> @@ -1079,6 +1156,29 @@ full_output:
> color_disable();
> }
>
> + if (rec->caller_id[0] != 0) {
if (*rec->caller_id)
> + size_t cid_len = strnlen(rec->caller_id, PID_CHARS_MAX);
Would be better to assume that the string zero is terminated? I guess
it's more robust for future code modifications.
> + ssize_t numspaces;
> + char strbuf[PID_CHARS_MAX];
> +
> + numspaces = ctl->caller_id_size - cid_len;
> +
> + memset(strbuf, 0, sizeof(strbuf));
> + if (numspaces > 0)
> + memset(strbuf, ' ', numspaces);
> +
> + if (ctl->json) {
> + ul_jsonwrt_value_s(&ctl->jfmt, "caller", rec->caller_id);
Here you assume it's zero terminated.
> + } else {
> + char cidbuf[PID_CHARS_MAX];
> +
> + memset(cidbuf, 0, sizeof(cidbuf));
Again, initialize, don't use memset().
> + sprintf(cidbuf, "[%s%s] ", strbuf, rec->caller_id);
Do we really need strbuf to create space before the string? What about
sprintf(cidbuf, "[%*s] ", numspaces, rec->caller_id);
Thanks!
Karel
--
Karel Zak <kzak@redhat.com>
http://karelzak.blogspot.com
prev parent reply other threads:[~2023-12-08 11:42 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-12-06 21:04 [PATCH] util-linux/sys-utils dmesg add PRINTK_CALLER id support Edward Chron
2023-12-08 11:42 ` Karel Zak [this message]
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=20231208114210.h2rmm7g44tjhjhco@ws.net.home \
--to=kzak@redhat.com \
--cc=colona@arista.com \
--cc=echron@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).