Re: [Qemu-devel] [PATCH v13 01/12] qcow2: Nicer variable names in qcow2_update_snapshot_refcount()

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2013 bytes --]

On 08.05.2017 17:36, Eric Blake wrote:
> On 05/08/2017 10:27 AM, Max Reitz wrote:
>> On 07.05.2017 02:05, Eric Blake wrote:
>>> In order to keep checkpatch happy when the next patch changes
>>> indentation, we first have to shorten some long lines.  The easiest
>>> approach is to use a new variable in place of
>>> 'offset & L2E_OFFSET_MASK', except that 'offset' is the best name
>>> for that variable.  Change '[old_]offset' to '[old_]entry' to
>>> make room.
>>>
>>> While touching things, also fix checkpatch warnings about unusual
>>> 'for' statements.
>>>
>>> Suggested by Max Reitz <mreitz@redhat.com>
>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>>
> 
>>> @@ -1117,20 +1118,22 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
>>>                  goto fail;
>>>              }
>>>
>>> -            for(j = 0; j < s->l2_size; j++) {
>>> +            for (j = 0; j < s->l2_size; j++) {
>>>                  uint64_t cluster_index;
>>> +                uint64_t offset;
>>
>> Actually, introducing entry and old_entry in this scope would have
>> sufficed, too, because they aren't used anywhere else. But nobody
>> actually cares (O:-)), so:
>>
>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>>
>>>
>>> -                offset = be64_to_cpu(l2_table[j]);
>>> -                old_offset = offset;
>>> -                offset &= ~QCOW_OFLAG_COPIED;
>>> +                entry = be64_to_cpu(l2_table[j]);
>>> +                old_entry = entry;
> 
> Other things I thought about, but didn't care enough to actually do:
> it's possible to reduce a line of code by either:
> 
> old_entry = entry = be64_to_cpu(l2_table[j]);
> entry &= ~QCOW_OFLAG_COPIED;
> 
> or by:
> 
> old_entry = be64_to_cpu(l2_table[j]);
> entry = old_entry & ~QCOW_OFLAG_COPIED;
> 
> for one less line of source.  You're welcome to make either such tweak
> if you care, but I'm not going to respin for just that ;)

No, I actually like the code as it is. :-)

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]