Re: [PATCH 1/2] hexdump: Convert the ascii boolean into a flag variable

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Mon, Aug 26, 2024 at 06:24:15PM +0200, Miquel Raynal wrote:
> The print_hex_dump() prototype is already quite long and there are
> already several hundred in-tree users. One argument is a boolean for
> enabling the ascii output. This is one way to format the buffer. We
> could think of other ways, which in this case might need other booleans
> to be enabled. In order to avoid messing several times with the
> prototype (and all the callers), let's switch to a 'flags'
> variable which can be easily extended.
> 
> There is no behavioral change intended.

...

>  extern void print_hex_dump(const char *level, const char *prefix_str,
>  			   int prefix_type, int rowsize, int groupsize,
> -			   const void *buf, size_t len, _Bool ascii);
> +			   const void *buf, size_t len,
> +			   unsigned int flags);

Why two lines? It fits perfectly even 80 limit.

...

>  static inline void print_hex_dump(const char *level, const char *prefix_str,
>  				  int prefix_type, int rowsize, int groupsize,
> -				  const void *buf, size_t len, _Bool ascii)
> +				  const void *buf, size_t len,
> +				  unsigned int flags)

Ditto. (And check entire conversion for theses unneeded new lines)

...

>  	print_hex_dump(KERN_ERR, "  ", DUMP_PREFIX_ADDRESS, 16,
> -			1, mpc, mpc->length, 1);
> +			1, mpc, mpc->length, DUMP_FLAG_ASCII);

While at it, I would reformat the code to be wrapped in more logical way, like

	print_hex_dump(KERN_ERR, "  ", DUMP_PREFIX_ADDRESS, 16, 1,
		       mpc, mpc->length, DUMP_FLAG_ASCII);

Also it fixes the broken indentation of the second line.
Same remark to the entire patch where it applies.

...

>  	pr_debug("Virtual Machine Save Area (VMSA):\n");
> -	print_hex_dump_debug("", DUMP_PREFIX_NONE, 16, 1, save, sizeof(*save), false);
> +	print_hex_dump_debug("", DUMP_PREFIX_NONE, 16, 1, save, sizeof(*save),
> +			     0);

Same, why two lines out of a sudden? If code already uses one line and it's
longer than 80, no need to wrap. The cases when it goes over 100 with new flag
name put there would make sense to wrap.

...

>  	if (copy_from_user(buf, (void __user *)(regs->pc & -16), sizeof(buf)) == 0) {
>  		print_hex_dump(KERN_INFO, " ", DUMP_PREFIX_NONE,
> -			       32, 1, buf, sizeof(buf), false);
> +			       32, 1, buf, sizeof(buf), 0);

I would assume you can even use one line now for this, but at bare minumum
"32, 1," part can be moved to the previous line. I dunno how to do this in
coccinelle in automatic way. In _this_ case it can be left as is, as there
is no logical issues (in comparison to one I pointed out above).


-- 
With Best Regards,
Andy Shevchenko