Re: [Qemu-block] [Qemu-devel] [PATCH v5 01/23] block: Allow NULL file for bdrv_get_block_status()

[Eric Blake]

On 10/10/2017 09:43 AM, Eric Blake wrote:

>>> ---
>>> v5: use second label for cleaner exit logic [John], use local_pnum
> 
>>> @@ -1811,16 +1811,19 @@ static int64_t coroutine_fn 
>>> bdrv_co_get_block_status(BlockDriverState *bs,
>>>      int64_t total_sectors;
>>>      int64_t n;
>>>      int64_t ret, ret2;
>>> +    BlockDriverState *local_file = NULL;
>>> +    int local_pnum = 0;
>>
>> I don't quite see what the point of local_pnum is if we assert anyway
>> that the real pnum is non-NULL.
> 
> I did it in parallel with fallout from John's review on v4:
> https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg06958.html
> 
> but since it wasn't specifically asked for, and is now getting
> questions, I'm fine with not having it in v6.

Okay, I re-read v4, and here's the comment (on 21/23) that led to my
experiment in v5 patch 1 with local_pnum:

https://lists.gnu.org/archive/html/qemu-devel/2017-10/msg00293.html

and I did argue:

https://lists.gnu.org/archive/html/qemu-devel/2017-10/msg00311.html

>> Is it asking for trouble to be updating pnum here before we undo our
>> alignment corrections? For readability reasons and preventing an
>> accidental context-based oopsy-daisy.
> 
> As in, write the code to make all calculations in a temporary, and then
> assign *pnum only at the end?  I suppose I can tweak the code along
> those lines, but I'm not sure it will make the end result any more legible.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature