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 <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.

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