| Message ID | 20210116220950.47078-2-timur@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | introduce DUMP_PREFIX_UNHASHED for hex dumps | expand |
On Sun, Jan 17, 2021 at 12:12 AM Timur Tabi <timur@kernel.org> wrote: (Hint: -v<n> to the git format-patch will create a versioned subject prefix for you automatically) > Hashed addresses are useless in hexdumps unless you're comparing > with other hashed addresses, which is unlikely. However, there's > no need to break existing code, so introduce a new prefix type > that prints unhashed addresses. Any user of this? (For the record, I don't see any other mail except this one) ... > enum { > DUMP_PREFIX_NONE, > DUMP_PREFIX_ADDRESS, > - DUMP_PREFIX_OFFSET > + DUMP_PREFIX_OFFSET, > + DUMP_PREFIX_UNHASHED, Since it's an address, I would like to group them together, i.e. put after DUMP_PREFIX_ADDRESS. Perhaps even add _ADDRESS to DUMP_PREFIX_UNHASHED, but this maybe too long. > }; ... > + * @prefix_type: controls whether prefix of an offset, hashed address, > + * unhashed address, or none is printed (%DUMP_PREFIX_OFFSET, > + * %DUMP_PREFIX_ADDRESS, %DUMP_PREFIX_UNHASHED, %DUMP_PREFIX_NONE) Yeah, exactly, here you use different ordering. ... > + * @prefix_type: controls whether prefix of an offset, hashed address, > + * unhashed address, or none is printed (%DUMP_PREFIX_OFFSET, > + * %DUMP_PREFIX_ADDRESS, %DUMP_PREFIX_UNHASHED, %DUMP_PREFIX_NONE) In both cases I would rather use colon and list one per line. What do you think? ... > + case DUMP_PREFIX_UNHASHED: Here is a third type of ordering, can you please be consistent? > case DUMP_PREFIX_ADDRESS: ... > + * @prefix_type: controls whether prefix of an offset, hashed address, > + * unhashed address, or none is printed (%DUMP_PREFIX_OFFSET, > + * %DUMP_PREFIX_ADDRESS, %DUMP_PREFIX_UNHASHED, %DUMP_PREFIX_NONE) As above. ... > + case DUMP_PREFIX_UNHASHED: As above. > case DUMP_PREFIX_ADDRESS:
On 1/18/21 4:03 AM, Andy Shevchenko wrote: > On Sun, Jan 17, 2021 at 12:12 AM Timur Tabi <timur@kernel.org> wrote: > > (Hint: -v<n> to the git format-patch will create a versioned subject > prefix for you automatically) I like to keep the version in the git repo itself so that I don't need to keep track of it separately, but thanks for the hint. I might use it somewhere else. >> Hashed addresses are useless in hexdumps unless you're comparing >> with other hashed addresses, which is unlikely. However, there's >> no need to break existing code, so introduce a new prefix type >> that prints unhashed addresses. > > Any user of this? (For the record, I don't see any other mail except this one) It's patch #2 of this set. They were all sent together. http://lkml.iu.edu/hypermail/linux/kernel/2101.2/00245.html Let me know what you think. >> DUMP_PREFIX_NONE, >> DUMP_PREFIX_ADDRESS, >> - DUMP_PREFIX_OFFSET >> + DUMP_PREFIX_OFFSET, >> + DUMP_PREFIX_UNHASHED, > > Since it's an address, I would like to group them together, i.e. put > after DUMP_PREFIX_ADDRESS. I didn't want to change the numbering of any existing enums, just in case there are users that accidentally hard-code the values. I'm trying to make this patch as unobtrusive as possible. > Perhaps even add _ADDRESS to DUMP_PREFIX_UNHASHED, but this maybe too long. I think DUMP_PREFIX_ADDRESS_UNHASHED is too long. >> + * @prefix_type: controls whether prefix of an offset, hashed address, >> + * unhashed address, or none is printed (%DUMP_PREFIX_OFFSET, >> + * %DUMP_PREFIX_ADDRESS, %DUMP_PREFIX_UNHASHED, %DUMP_PREFIX_NONE) > > Yeah, exactly, here you use different ordering. That's because it's a comment. >> + * @prefix_type: controls whether prefix of an offset, hashed address, >> + * unhashed address, or none is printed (%DUMP_PREFIX_OFFSET, >> + * %DUMP_PREFIX_ADDRESS, %DUMP_PREFIX_UNHASHED, %DUMP_PREFIX_NONE) > > In both cases I would rather use colon and list one per line. What do you think? Hmmmm.... if I'm going to change the patch anyway, sure. >> + case DUMP_PREFIX_UNHASHED: > > Here is a third type of ordering, can you please be consistent? > >> case DUMP_PREFIX_ADDRESS: Fair enough.
On Mon, Jan 18, 2021 at 09:57:55AM -0600, Timur Tabi wrote: > On 1/18/21 4:03 AM, Andy Shevchenko wrote: > > On Sun, Jan 17, 2021 at 12:12 AM Timur Tabi <timur@kernel.org> wrote: ... > > Any user of this? (For the record, I don't see any other mail except this one) > It's patch #2 of this set. I haven't got that one. > They were all sent together. Apparently not to me. > http://lkml.iu.edu/hypermail/linux/kernel/2101.2/00245.html > > Let me know what you think. Makes sense. Hint: use lore.kernel.org references as they are much better in terms of provided features and patch representation. ... > > > DUMP_PREFIX_NONE, > > > DUMP_PREFIX_ADDRESS, > > > - DUMP_PREFIX_OFFSET > > > + DUMP_PREFIX_OFFSET, > > > + DUMP_PREFIX_UNHASHED, > > > > Since it's an address, I would like to group them together, i.e. put > > after DUMP_PREFIX_ADDRESS. > > I didn't want to change the numbering of any existing enums, just in case > there are users that accidentally hard-code the values. I'm trying to make > this patch as unobtrusive as possible. But isn't it good to expose those issues (and fix them)? ... > > Perhaps even add _ADDRESS to DUMP_PREFIX_UNHASHED, but this maybe too > long. > > I think DUMP_PREFIX_ADDRESS_UNHASHED is too long. What about introducing new two like these: DUMP_PREFIX_OFFSET, DUMP_PREFIX_ADDRESS, DUMP_PREFIX_ADDR_UNHASHED, DUMP_PREFIX_ADDR_HASHED, and allow people step-by-step move to them?
On 1/18/21 11:14 AM, Andy Shevchenko wrote: > But isn't it good to expose those issues (and fix them)? I suppose. >>> Perhaps even add _ADDRESS to DUMP_PREFIX_UNHASHED, but this maybe too >> long. >> >> I think DUMP_PREFIX_ADDRESS_UNHASHED is too long. > What about introducing new two like these: > > DUMP_PREFIX_OFFSET, > DUMP_PREFIX_ADDRESS, > DUMP_PREFIX_ADDR_UNHASHED, > DUMP_PREFIX_ADDR_HASHED, I think we're approaching bike-shedding. DUMP_PREFIX_ADDR_HASHED and DUMP_PREFIX_ADDRESS are the same thing. I don't want people to have to move from DUMP_PREFIX_ADDRESS to some other enum for no change in functionality. I'm willing to rearrange the code so that it's enumerated more consistently, but I don't think there's anything wrong with DUMP_PREFIX_UNHASHED. It's succinct and clear.
diff --git a/fs/seq_file.c b/fs/seq_file.c index 03a369ccd28c..b5b49a855894 100644 --- a/fs/seq_file.c +++ b/fs/seq_file.c @@ -864,6 +864,9 @@ void seq_hex_dump(struct seq_file *m, const char *prefix_str, int prefix_type, remaining -= rowsize; switch (prefix_type) { + case DUMP_PREFIX_UNHASHED: + seq_printf(m, "%s%px: ", prefix_str, ptr + i); + break; case DUMP_PREFIX_ADDRESS: seq_printf(m, "%s%p: ", prefix_str, ptr + i); break; diff --git a/include/linux/printk.h b/include/linux/printk.h index fe7eb2351610..d3c08095a9a3 100644 --- a/include/linux/printk.h +++ b/include/linux/printk.h @@ -567,7 +567,8 @@ extern const struct file_operations kmsg_fops; enum { DUMP_PREFIX_NONE, DUMP_PREFIX_ADDRESS, - DUMP_PREFIX_OFFSET + DUMP_PREFIX_OFFSET, + DUMP_PREFIX_UNHASHED, }; extern int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize, char *linebuf, size_t linebuflen, @@ -612,8 +613,9 @@ static inline void print_hex_dump_debug(const char *prefix_str, int prefix_type, * print_hex_dump_bytes - shorthand form of print_hex_dump() with default params * @prefix_str: string to prefix each line with; * caller supplies trailing spaces for alignment if desired - * @prefix_type: controls whether prefix of an offset, address, or none - * is printed (%DUMP_PREFIX_OFFSET, %DUMP_PREFIX_ADDRESS, %DUMP_PREFIX_NONE) + * @prefix_type: controls whether prefix of an offset, hashed address, + * unhashed address, or none is printed (%DUMP_PREFIX_OFFSET, + * %DUMP_PREFIX_ADDRESS, %DUMP_PREFIX_UNHASHED, %DUMP_PREFIX_NONE) * @buf: data blob to dump * @len: number of bytes in the @buf * diff --git a/lib/hexdump.c b/lib/hexdump.c index 9301578f98e8..b5acfc4168a8 100644 --- a/lib/hexdump.c +++ b/lib/hexdump.c @@ -211,8 +211,9 @@ EXPORT_SYMBOL(hex_dump_to_buffer); * @level: kernel log level (e.g. KERN_DEBUG) * @prefix_str: string to prefix each line with; * caller supplies trailing spaces for alignment if desired - * @prefix_type: controls whether prefix of an offset, address, or none - * is printed (%DUMP_PREFIX_OFFSET, %DUMP_PREFIX_ADDRESS, %DUMP_PREFIX_NONE) + * @prefix_type: controls whether prefix of an offset, hashed address, + * unhashed address, or none is printed (%DUMP_PREFIX_OFFSET, + * %DUMP_PREFIX_ADDRESS, %DUMP_PREFIX_UNHASHED, %DUMP_PREFIX_NONE) * @rowsize: number of bytes to print per line; must be 16 or 32 * @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1) * @buf: data blob to dump @@ -256,6 +257,10 @@ void print_hex_dump(const char *level, const char *prefix_str, int prefix_type, linebuf, sizeof(linebuf), ascii); switch (prefix_type) { + case DUMP_PREFIX_UNHASHED: + printk("%s%s%px: %s\n", + level, prefix_str, ptr + i, linebuf); + break; case DUMP_PREFIX_ADDRESS: printk("%s%s%p: %s\n", level, prefix_str, ptr + i, linebuf); diff --git a/lib/seq_buf.c b/lib/seq_buf.c index 707453f5d58e..017c4d7e93f1 100644 --- a/lib/seq_buf.c +++ b/lib/seq_buf.c @@ -335,8 +335,9 @@ int seq_buf_to_user(struct seq_buf *s, char __user *ubuf, int cnt) * @s: seq_buf descriptor * @prefix_str: string to prefix each line with; * caller supplies trailing spaces for alignment if desired - * @prefix_type: controls whether prefix of an offset, address, or none - * is printed (%DUMP_PREFIX_OFFSET, %DUMP_PREFIX_ADDRESS, %DUMP_PREFIX_NONE) + * @prefix_type: controls whether prefix of an offset, hashed address, + * unhashed address, or none is printed (%DUMP_PREFIX_OFFSET, + * %DUMP_PREFIX_ADDRESS, %DUMP_PREFIX_UNHASHED, %DUMP_PREFIX_NONE) * @rowsize: number of bytes to print per line; must be 16 or 32 * @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1) * @buf: data blob to dump @@ -374,6 +375,10 @@ int seq_buf_hex_dump(struct seq_buf *s, const char *prefix_str, int prefix_type, linebuf, sizeof(linebuf), ascii); switch (prefix_type) { + case DUMP_PREFIX_UNHASHED: + ret = seq_buf_printf(s, "%s%px: %s\n", + prefix_str, ptr + i, linebuf); + break; case DUMP_PREFIX_ADDRESS: ret = seq_buf_printf(s, "%s%p: %s\n", prefix_str, ptr + i, linebuf);