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 <ebl...@redhat.com>
> ---
> 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
