On 1/28/21 8:07 AM, Peter Lieven wrote: > Signed-off-by: Peter Lieven <p...@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