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 <address@hidden> 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 <address@hidden>