On 09.08.2016 20:39, Reda Sallahi wrote: > Hi Max, > Thanks for the review! > > On 8/8/16, Max Reitz <[email protected]> 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
