Re: [Qemu-devel] [PATCH 2/2] qemu-img: Add dd seek= option

[Max Reitz]

On 2018-08-15 04:56, Eric Blake wrote:
> For feature parity with dd, we want to be able to specify
> the offset within the output file, just as we can specify
> the offset for the input (in particular, this makes copying
> a subset range of guest-visible bytes from one file to
> another much easier).

In my opinion, we do not want feature parity with dd.  What we do want
is feature parity with convert.

> The code style for 'qemu-img dd' was pretty hard to read;
> unfortunately this patch focuses only on adding the new
> feature in the existing style rather than trying to improve
> the overall flow, other than switching octal constants to
> hex.  Oh well.

No, the real issue is that dd is still not implemented just as a
frontend to convert.  Which it should be.  I'm not sure dd was a very
good idea from the start, and now it should ideally be a frontend to
convert.

(My full opinion on the matter: dd has a horrible interface.  I don't
quite see why we replicated that inside qemu-img.  Also, if you want to
use dd, why not use qemu-nbd + Linux nbd device + real dd?)

((That gave me a good idea.  Actually, it's probably not such a good
idea, but I guess I'll do it in my spare time anyway.  A qemu-img fuse
might be nice which represents an image as a raw image at some mount
point.  Benefits over qemu-nbd: (1) You don't need root, (2) you don't
need to type modprobe nbd.))

> Also, switch the test to use an offset of 0 instead of 1,
> to test skip= and seek= on their own; as it is, this is
> effectively quadrupling the test runtime, which starts
> to make this test borderline on whether it should still
> belong to './check -g quick'.  And I didn't bother to
> reindent the test shell code for the new nested loop.

In my opinion, it should no longer belong to quick.  It takes 8 s on my
tmpfs.  My border is somewhere around 2 or 3; and I haven't yet decided
whether that's on tmpfs or SSD.

> Signed-off-by: Eric Blake <address@hidden>
> ---
>  qemu-img.c                 |  41 ++++--
>  tests/qemu-iotests/160     |  12 +-
>  tests/qemu-iotests/160.out | 304 
> +++++++++++++++++++++++++++++++++++++++++++--
>  3 files changed, 336 insertions(+), 21 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index d72f0f0ec94..ee01a18f331 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c

[...]

> @@ -4574,7 +4592,14 @@ static int img_dd(int argc, char **argv)
>          size = dd.count * in.bsz;
>      }
> 
> -    qemu_opt_set_number(opts, BLOCK_OPT_SIZE, size, &error_abort);
> +    if (dd.flags & C_SEEK && out.offset * out.bsz > INT64_MAX - size) {

What about overflows in out.offset * out.bsz?

> +        error_report("Seek too large for '%s'", out.filename);
> +        ret = -1;
> +        goto out;

Real dd doesn't seem to error out (it just reports an error).  I don't
know whether that makes any difference, though.

The test looks good to me.

Max

> +    }
> +    seek = out.offset * out.bsz;
> +
> +    qemu_opt_set_number(opts, BLOCK_OPT_SIZE, size + seek, &error_abort);
>      end = size + in_pos;
> 
>      ret = bdrv_create(drv, out.filename, opts, &local_err);

signature.asc
Description: OpenPGP digital signature