Re: [Qemu-block] [PATCH] Fixed dump_buffer function parameter offset does not take effect

[John Snow]

Hi! Thank you for this patch.

Some notes follow:

On 4/4/19 7:46 AM, lichun wrote:
> Signed-off-by: lichun <address@hidden>

You should write a commit message explaining the problem being fixed by
this patch, even if it's very brief.

In the future, try wording subject lines in terms of what the patch
"does" instead of what it "did"; i.e.:

"Fix dump_buffer function so that the offset parameter takes effect"
is preferable to
"Fixed dump_buffer function parameter offset does not take effect"

> ---
>  qemu-io-cmds.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
> index 09750a2..8d93dc6 100644
> --- a/qemu-io-cmds.c
> +++ b/qemu-io-cmds.c
> @@ -357,7 +357,7 @@ static void dump_buffer(const void *buffer, int64_t 
> offset, int64_t len)
>      int j;
>      const uint8_t *p;
>  
> -    for (i = 0, p = buffer; i < len; i += 16) {
> +    for (i = 0, p = buffer + offset; i < len; i += 16) {
>          const uint8_t *s = p;
>  
>          printf("%08" PRIx64 ":  ", offset + i);
> 

At a glance I am not sure that this patch is correct, it looks to me as
if callers are expecting the entire buffer to be dumped and the offset
is the offset of *this buffer* relative to some reference point (e.g.
the disk being read from.)

Can you point me to examples in which this is obviously wrong?