Re: [Qemu-block] [PATCH v6] qemu-img: add the 'dd' subcommand

[Max Reitz]

On 09.08.2016 20:39, Reda Sallahi wrote:
> Hi Max,
> Thanks for the review!
> 
> On 8/8/16, Max Reitz  wrote:
>> On 25.07.2016 07:58, Reda Sallahi wrote:
>>> +if (in->bsz == 0) {
>>> +error_report("invalid number: '%s'", arg);
>>
>> It's not an invalid number, it's just an invalid block size. In my
>> understanding, those are two different things.
> 
> I was trying to have the same error message as dd(1) for this.

OK. I hope nobody is really relying on dd emitting exactly this error
message, though. :-)

>>> +tmp = strchr(arg, '=');
>>
>> FYI, strtok() is a neat function for exactly this. I'm not saying you
>> need to use it, though.
> 
> For something as simple I would rather avoid using strtok().

Yep, that's fine.

>>> +for (; incount < size; block_count++) {
>>
>> Please do not initialize incount above but here. Having to jump more
>> than a hundred lines up to find out what a variable is set to makes code
>> very hard to read.
>>
>> Also, I'd rename incount to in_offset or something, because "incount" to
>> me sounds like it counts a number of blocks, whereas it actually counts
>> a number of bytes, independently of the block size.
>>
> 
> There is already in.offset ([PATCH v4] qemu-img: add skip option to dd)
> so it will just be confusing but if you have a better name I will be glad to
> change it.

Hm. How about "in_position"?

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v6] qemu-img: add the 'dd' subcommand

[Reda Sallahi]

Hi Max,
Thanks for the review!

On 8/8/16, Max Reitz  wrote:
> On 25.07.2016 07:58, Reda Sallahi wrote:
>> +if (in->bsz == 0) {
>> +error_report("invalid number: '%s'", arg);
>
> It's not an invalid number, it's just an invalid block size. In my
> understanding, those are two different things.

I was trying to have the same error message as dd(1) for this.

>> +tmp = strchr(arg, '=');
>
> FYI, strtok() is a neat function for exactly this. I'm not saying you
> need to use it, though.

For something as simple I would rather avoid using strtok().

>> +for (; incount < size; block_count++) {
>
> Please do not initialize incount above but here. Having to jump more
> than a hundred lines up to find out what a variable is set to makes code
> very hard to read.
>
> Also, I'd rename incount to in_offset or something, because "incount" to
> me sounds like it counts a number of blocks, whereas it actually counts
> a number of bytes, independently of the block size.
>

There is already in.offset ([PATCH v4] qemu-img: add skip option to dd)
so it will just be confusing but if you have a better name I will be glad to
change it.

-- 
Reda 

Re: [Qemu-block] [PATCH v6] qemu-img: add the 'dd' subcommand

[Kevin Wolf]

Am 25.07.2016 um 07:58 hat Reda Sallahi geschrieben:
> This patch adds a basic dd subcommand analogous to dd(1) to qemu-img.
> 
> For the start, this implements the bs, if, of and count options and requires
> both if and of to be specified (no stdin/stdout if not specified) and doesn't
> support tty, pipes, etc.
> 
> The image format must be specified with -O for the output if the raw format
> is not the intended one.
> 
> Two tests are added to test qemu-img dd.
> 
> Signed-off-by: Reda Sallahi 

> +/*
> + * get_size() was needed for the size syntax dd(1) supports which is
> + * different from qemu_strtosz_suffix()
> + *
> + */

(Excess empty line in comment)

Is it really a good idea to stay consistent with dd when this makes the
subcommand inconsistent with all other size specifications in qemu? If I
understand correctly, the only difference is that some suffixes wouldn't
be supported, so you would get an error message rather than surprising
behaviour. I would consider that fine and probably preferrable to adding
another size parser. Nobody uses 'c', 'w' or 'b' anyway.

Kevin

Re: [Qemu-block] [PATCH v6] qemu-img: add the 'dd' subcommand

[Max Reitz]

On 25.07.2016 07:58, Reda Sallahi wrote:
> This patch adds a basic dd subcommand analogous to dd(1) to qemu-img.
> 
> For the start, this implements the bs, if, of and count options and requires
> both if and of to be specified (no stdin/stdout if not specified) and doesn't
> support tty, pipes, etc.
> 
> The image format must be specified with -O for the output if the raw format
> is not the intended one.
> 
> Two tests are added to test qemu-img dd.
> 
> Signed-off-by: Reda Sallahi 
> ---
> Changes from v5:
> * Add dd sections on qemu-img.texi.
> Changes from v4:
> * Fix the exit status.
> Changes from v3:
> * Delete an unused (so far) field in DdIo.
> Changes from v2:
> * Add copyright headers to new files.
> Changes from v1:
> * Removal of dead code.
> * Fix a memory leak.
> * Complete the cleanup function in the test cases.
> 
>  qemu-img-cmds.hx |   6 +
>  qemu-img.c   | 363 
> ++-
>  qemu-img.texi|  25 +++
>  tests/qemu-iotests/158   |  68 
>  tests/qemu-iotests/158.out   |  15 ++
>  tests/qemu-iotests/159   |  70 
>  tests/qemu-iotests/159.out   |  87 ++
>  tests/qemu-iotests/common.filter |   9 +
>  tests/qemu-iotests/group |   2 +
>  9 files changed, 644 insertions(+), 1 deletion(-)
>  create mode 100755 tests/qemu-iotests/158
>  create mode 100644 tests/qemu-iotests/158.out
>  create mode 100755 tests/qemu-iotests/159
>  create mode 100644 tests/qemu-iotests/159.out
> 
> diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
> index 7e95b2d..03bdd7a 100644
> --- a/qemu-img-cmds.hx
> +++ b/qemu-img-cmds.hx
> @@ -45,6 +45,12 @@ STEXI
>  @item convert [--object @var{objectdef}] [--image-opts] [-c] [-p] [-q] [-n] 
> [-f @var{fmt}] [-t @var{cache}] [-T @var{src_cache}] [-O @var{output_fmt}] 
> [-o @var{options}] [-s @var{snapshot_id_or_name}] [-l @var{snapshot_param}] 
> [-S @var{sparse_size}] @var{filename} [@var{filename2} [...]] 
> @var{output_filename}
>  ETEXI
>  
> +DEF("dd", img_dd,
> +"dd [--image-opts] [-f fmt] [-O output_fmt] [bs=block_size] 
> [count=blocks] if=input of=output")
> +STEXI
> +@item dd [--image-opts] [-f @var{fmt}] [-O @var{output_fmt}] 
> [bs=@var{block_size}] [count=@var{blocks}] if=@var{input} of=@var{output}
> +ETEXI
> +
>  DEF("info", img_info,
>  "info [--object objectdef] [--image-opts] [-f fmt] [--output=ofmt] 
> [--backing-chain] filename")
>  STEXI
> diff --git a/qemu-img.c b/qemu-img.c
> index 2e40e1f..498626b 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -166,7 +166,14 @@ static void QEMU_NORETURN help(void)
> "Parameters to compare subcommand:\n"
> "  '-f' first image format\n"
> "  '-F' second image format\n"
> -   "  '-s' run in Strict mode - fail on different image size or 
> sector allocation\n";
> +   "  '-s' run in Strict mode - fail on different image size or 
> sector allocation\n"
> +   "\n"
> +   "Parameters to dd subcommand:\n"
> +   "  'bs=BYTES' read and write up to BYTES bytes at a time "
> +   "(default: 512)\n"
> +   "  'count=N' copy only N input blocks\n"
> +   "  'if=FILE' read from FILE\n"
> +   "  'of=FILE' write to FILE\n";
>  
>  printf("%s\nSupported formats:", help_msg);
>  bdrv_iterate_format(format_print, NULL);
> @@ -3794,6 +3801,360 @@ out:
>  return 0;
>  }
>  
> +#define C_BS  01
> +#define C_COUNT   02
> +#define C_IF  04
> +#define C_OF  010

Not sure why you used octal here, but since two people already gave
their R-b, I guess it at least has some novelty value. ;-)

Also, I think I'd personally use an enum for this, but that is very much
personal taste.

> +
> +struct DdInfo {
> +unsigned int flags;
> +size_t count;

I'd strongly suggest using uint64_t because the width of size_t depends
on the platform.

> +};
> +
> +struct DdIo {
> +size_t bsz;/* Block size */

I wouldn't use size_t here because of the above reason. However, I would
not use uint64_t here, since you will be passing this value to g_new()
and also to blk_pread(), so I'd use the type of the latter's parameter,
which is int.

> +char *filename;
> +uint8_t *buf;
> +};
> +
> +struct DdOpts {
> +const char *name;
> +int (*f)(const char *, struct DdIo *, struct DdIo *, struct DdInfo *);
> +unsigned int flag;
> +};
> +
> +/*
> + * get_size() was needed for the size syntax dd(1) supports which is
> + * different from qemu_strtosz_suffix()
> + *
> + */
> +static size_t get_size(const char *str)

This function should return a uint64_t, otherwise you won't be able to
go beyond 4 GB on 32 bit machines.

Also, I'd strongly suggest giving this function an Error **errp
parameter, because I really don't like doing internal error handling
using errno.

> +{
> +/* XXX: handle {k,m,g}B notations */
> +unsigned long num;
> +size_t 

Re: [Qemu-block] [PATCH v6] qemu-img: add the 'dd' subcommand

[Stefan Hajnoczi]

On Mon, Jul 25, 2016 at 6:58 AM, Reda Sallahi  wrote:
> This patch adds a basic dd subcommand analogous to dd(1) to qemu-img.
>
> For the start, this implements the bs, if, of and count options and requires
> both if and of to be specified (no stdin/stdout if not specified) and doesn't
> support tty, pipes, etc.
>
> The image format must be specified with -O for the output if the raw format
> is not the intended one.
>
> Two tests are added to test qemu-img dd.
>
> Signed-off-by: Reda Sallahi 
> ---
> Changes from v5:
> * Add dd sections on qemu-img.texi.
> Changes from v4:
> * Fix the exit status.
> Changes from v3:
> * Delete an unused (so far) field in DdIo.
> Changes from v2:
> * Add copyright headers to new files.
> Changes from v1:
> * Removal of dead code.
> * Fix a memory leak.
> * Complete the cleanup function in the test cases.
>
>  qemu-img-cmds.hx |   6 +
>  qemu-img.c   | 363 
> ++-
>  qemu-img.texi|  25 +++
>  tests/qemu-iotests/158   |  68 
>  tests/qemu-iotests/158.out   |  15 ++
>  tests/qemu-iotests/159   |  70 
>  tests/qemu-iotests/159.out   |  87 ++
>  tests/qemu-iotests/common.filter |   9 +
>  tests/qemu-iotests/group |   2 +
>  9 files changed, 644 insertions(+), 1 deletion(-)
>  create mode 100755 tests/qemu-iotests/158
>  create mode 100644 tests/qemu-iotests/158.out
>  create mode 100755 tests/qemu-iotests/159
>  create mode 100644 tests/qemu-iotests/159.out

Reviewed-by: Stefan Hajnoczi 

Re: [Qemu-block] [PATCH v6] qemu-img: add the 'dd' subcommand

[Fam Zheng]

On Mon, 07/25 07:58, Reda Sallahi wrote:
> This patch adds a basic dd subcommand analogous to dd(1) to qemu-img.
> 
> For the start, this implements the bs, if, of and count options and requires
> both if and of to be specified (no stdin/stdout if not specified) and doesn't
> support tty, pipes, etc.
> 
> The image format must be specified with -O for the output if the raw format
> is not the intended one.
> 
> Two tests are added to test qemu-img dd.
> 
> Signed-off-by: Reda Sallahi 

Reviewed-by: Fam Zheng