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@xxxxxxxxxx> http://karelzak.blogspot.com
