[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v6] qemu-img: add the 'dd' subcommand

From: Max Reitz
Subject: Re: [Qemu-devel] [PATCH v6] qemu-img: add the 'dd' subcommand
Date: Tue, 9 Aug 2016 22:43:53 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0

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"?


Attachment: signature.asc
Description: OpenPGP digital signature

reply via email to

[Prev in Thread] Current Thread [Next in Thread]