Hi Max, Thanks for the review! On 8/8/16, Max Reitz <mre...@redhat.com> 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 <fullma...@gmail.com>