Re: [PATCH 1/2] [v2] lib/hexdump: introduce DUMP_PREFIX_UNHASHED for unhashed addresses

[Timur Tabi]

On 1/18/21 4:03 AM, Andy Shevchenko wrote:
> On Sun, Jan 17, 2021 at 12:12 AM Timur Tabi <timur@xxxxxxxxxx> 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.