Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 6 Nov 2019 03:53:17 -0500
From:   Steven Rostedt <rostedt@goodmis.org>
To:     Piotr Maziarz <piotrx.maziarz@linux.intel.com>
Cc:     linux-kernel@vger.kernel.org, mingo@redhat.com,
        andriy.shevchenko@intel.com, cezary.rojewski@intel.com,
        gustaw.lewandowski@intel.com
Subject: Re: [PATCH 1/2] seq_buf: Add printing formatted hex dumps
Message-ID: <20191106035317.7558e47e@grimm.local.home>
In-Reply-To: <1573021660-30540-1-git-send-email-piotrx.maziarz@linux.intel.com>
References: <1573021660-30540-1-git-send-email-piotrx.maziarz@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed,  6 Nov 2019 07:27:39 +0100
Piotr Maziarz <piotrx.maziarz@linux.intel.com> wrote:

> Provided function is an analogue of print_hex_dump().
> 
> Implementing this function in seq_buf allows using for multiple
> purposes (e.g. for tracing) and therefore prevents from code duplication
> in every layer that uses seq_buf.
> 
> print_hex_dump() is an essential part of logging data to dmesg. Adding
> similar capability for other purposes is beneficial to all users.

Can you add to the change log an example output of print_hex_dump().

It makes it easier for reviewers to know if what is implemented matches
what is expected.

> 
> Signed-off-by: Piotr Maziarz <piotrx.maziarz@linux.intel.com>
> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
> ---
>  include/linux/seq_buf.h |  3 +++
>  lib/seq_buf.c           | 38 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 41 insertions(+)
> 
> diff --git a/include/linux/seq_buf.h b/include/linux/seq_buf.h
> index aa5deb0..fb0205d 100644
> --- a/include/linux/seq_buf.h
> +++ b/include/linux/seq_buf.h
> @@ -125,6 +125,9 @@ extern int seq_buf_putmem(struct seq_buf *s, const void *mem, unsigned int len);
>  extern int seq_buf_putmem_hex(struct seq_buf *s, const void *mem,
>  			      unsigned int len);
>  extern int seq_buf_path(struct seq_buf *s, const struct path *path, const char *esc);
> +extern int seq_buf_hex_dump(struct seq_buf *s, const char *prefix_str,
> +			    int prefix_type, int rowsize, int groupsize,
> +			    const void *buf, size_t len, bool ascii);
>  
>  #ifdef CONFIG_BINARY_PRINTF
>  extern int
> diff --git a/lib/seq_buf.c b/lib/seq_buf.c
> index bd807f5..0509706 100644
> --- a/lib/seq_buf.c
> +++ b/lib/seq_buf.c
> @@ -328,3 +328,41 @@ int seq_buf_to_user(struct seq_buf *s, char __user *ubuf, int cnt)
>  	s->readpos += cnt;
>  	return cnt;
>  }
> +

Requires a kernel doc header here.

> +int seq_buf_hex_dump(struct seq_buf *s, const char *prefix_str, int prefix_type,
> +		     int rowsize, int groupsize,
> +		     const void *buf, size_t len, bool ascii)
> +{
> +	const u8 *ptr = buf;
> +	int i, linelen, remaining = len;
> +	unsigned char linebuf[32 * 3 + 2 + 32 + 1];

What do the above magic numbers mean? Should have a comment explaining
why you picked those numbers and created the length this way.

Also the preferred method of declarations is to order it by longest
first. That is, linebuf, followed by the ints, followed by ptr.


> +	int ret;
> +
> +	if (rowsize != 16 && rowsize != 32)
> +		rowsize = 16;
> +
> +	for (i = 0; i < len; i += rowsize) {
> +		linelen = min(remaining, rowsize);
> +		remaining -= rowsize;

Probably should make the above:

		remaining -= linelen;

Yeah, what you have works, but it makes a reviewer worry about using
remaining later and having it negative.

> +
> +		hex_dump_to_buffer(ptr + i, linelen, rowsize, groupsize,
> +				   linebuf, sizeof(linebuf), ascii);
> +
> +		switch (prefix_type) {
> +		case DUMP_PREFIX_ADDRESS:

I'm curious to know what uses the above type? By default, today,
pointers are pretty much obfuscated, and that will show up here too.

-- Steve

> +			ret = seq_buf_printf(s, "%s%p: %s\n",
> +			       prefix_str, ptr + i, linebuf);
> +			break;
> +		case DUMP_PREFIX_OFFSET:
> +			ret = seq_buf_printf(s, "%s%.8x: %s\n",
> +					     prefix_str, i, linebuf);
> +			break;
> +		default:
> +			ret = seq_buf_printf(s, "%s%s\n", prefix_str, linebuf);
> +			break;
> +		}
> +		if (ret)
> +			return ret;
> +	}
> +	return 0;
> +}