Re: [PATCH] qemu-img: add seek and -n option to dd command

[Eric Blake]

On 1/28/21 8:07 AM, Peter Lieven wrote:
> Signed-off-by: Peter Lieven <pl@kamp.de>

Your commit message says 'what', but not 'why'.  Generally, the one-line
'what' works well as the subject line, but you want the commit body to
give an argument why your patch should be applied, rather than blank.

Here's the last time we tried to improve qemu-img dd:
https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg02618.html

where I also proposed adding seek=, and fixing skip= with count=.  Your
patch does not do the latter.  But the bigger complaint back then was
that 'qemu-img copy' should be able to do everything, and that qemu-img
dd should then just be a thin shim around 'qemu-img copy', rather than
having two parallel projects that diverge in their implementations.

Your patch does not have the typical '---' divider and diffstat between
the commit message and the patch proper; this may be a factor of which
git packages you have installed, but having the diffstat present makes
it easier to see at a glance what your patch touches without reading the
entire email.  I had to go hunting to learn if you added iotest coverage
of this new feature...

...and the answer was no, you didn't.  You'll need to add that in v2
(see the link to my earlier attempt at modifying dd for an example).

> 
> diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
> index b615aa8419..7d4564c2b8 100644
> --- a/docs/tools/qemu-img.rst
> +++ b/docs/tools/qemu-img.rst
> @@ -209,6 +209,10 @@ Parameters to dd subcommand:
>  
>  .. program:: qemu-img-dd
>  
> +.. option:: -n
> +
> +  Skip the creation of the output file
> +
>  .. option:: bs=BLOCK_SIZE
>  
>    Defines the block size
> @@ -229,6 +233,10 @@ Parameters to dd subcommand:
>  
>    Sets the number of input blocks to skip
>  
> +.. option:: sseek=BLOCKS

Typo: seek=

> +
> +  Sets the number of blocks to seek into the output
> +
>  Parameters to snapshot subcommand:
>  
>  .. program:: qemu-img-snapshot
> diff --git a/qemu-img.c b/qemu-img.c
> index 8597d069af..d7f390e382 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -213,12 +213,17 @@ static void QEMU_NORETURN help(void)
>             "  '-s' run in Strict mode - fail on different image size or 
> sector allocation\n"
>             "\n"
>             "Parameters to dd subcommand:\n"
> +           "  '-n' skips the target volume creation (useful if the volume is 
> created\n"
> +           "       prior to running qemu-img). Note that he behaviour is not 
> identical to\n"

the

> +           "       original dd option conv=nocreat. The output is neither 
> truncated nor\n"
> +           "       is it possible to write past the end of an existing 
> file.\n"

and hence why you spell it -n rather than the more dd-like conv=nocreat.
 We absolutely want a different spelling to emphasize the different
behavior.

Is it worth splitting this patch in two parts, one for -n, the other for
seek=?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org