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

[John Snow]

On 10/10/2017 03:00 PM, Eric Blake wrote:
> 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
> 

Well, Kevin's the boss :D

>>> 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.
>