On 09.08.2016 20:39, Reda Sallahi wrote: > 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.
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