Re: [PATCH 1/2] dump: simplify a bit kdump get_next_page()

[David Hildenbrand]

On 26.08.22 11:56, Marc-André Lureau wrote:
> Hi
> 
> On Fri, Aug 26, 2022 at 1:45 PM David Hildenbrand <david@redhat.com> wrote:
>>
>> On 25.08.22 15:21, marcandre.lureau@redhat.com wrote:
>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>
>>> This should be functionally equivalent, but slightly easier to read,
>>> with simplified paths and checks at the end of the function.
>>>
>>> The following patch is a major rewrite to get rid of the assert().
>>>
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> ---
>>>  dump/dump.c | 30 ++++++++++++------------------
>>>  1 file changed, 12 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/dump/dump.c b/dump/dump.c
>>> index 4d9658ffa2..18f06cffe2 100644
>>> --- a/dump/dump.c
>>> +++ b/dump/dump.c
>>> @@ -1107,37 +1107,31 @@ static bool get_next_page(GuestPhysBlock 
>>> **blockptr, uint64_t *pfnptr,
>>>      uint8_t *buf;
>>>
>>>      /* block == NULL means the start of the iteration */
>>> -    if (!block) {
>>> -        block = QTAILQ_FIRST(&s->guest_phys_blocks.head);
>>> -        *blockptr = block;
>>> -        assert((block->target_start & ~target_page_mask) == 0);
>>> -        assert((block->target_end & ~target_page_mask) == 0);
>>> -        *pfnptr = dump_paddr_to_pfn(s, block->target_start);
>>> -        if (bufptr) {
>>> -            *bufptr = block->host_addr;
>>> -        }
>>> -        return true;
>>
>>
>> Instead of the "return true" we'll now do take the  "if ((addr >=
>> block->target_start) &&" path below I guess, always ending up with
>> essentially "buf = buf;" because addr == block->target_start.
>>
>> I guess that's fine.
>>
>>> +    if (block == NULL) {
>>
>> What's wrong with keeping the "if (!block) {" ? :)
> 
> That's just to be consistent with the comment above.
> 
>>
>>> +        *blockptr = block = QTAILQ_FIRST(&s->guest_phys_blocks.head);
>>
>> Another unnecessary change.
>>
>>> +        addr = block->target_start;
>>> +    } else {
>>> +        addr = dump_pfn_to_paddr(s, *pfnptr + 1);
>>>      }
>>> -
>>> -    *pfnptr = *pfnptr + 1;
>>> -    addr = dump_pfn_to_paddr(s, *pfnptr);
>>> +    assert(block != NULL);
>>>
>>>      if ((addr >= block->target_start) &&
>>>          (addr + s->dump_info.page_size <= block->target_end)) {
>>>          buf = block->host_addr + (addr - block->target_start);
>>>      } else {
>>>          /* the next page is in the next block */
>>> -        block = QTAILQ_NEXT(block, next);
>>> -        *blockptr = block;
>>> +        *blockptr = block = QTAILQ_NEXT(block, next);
>>
>> Another unnecessary change. (avoiding these really eases review, because
>> the focus is then completely on the actual code changes)
>>
>>>          if (!block) {
>>>              return false;
>>>          }
>>> -        assert((block->target_start & ~target_page_mask) == 0);
>>> -        assert((block->target_end & ~target_page_mask) == 0);
>>> -        *pfnptr = dump_paddr_to_pfn(s, block->target_start);
>>> +        addr = block->target_start;
>>>          buf = block->host_addr;
>>>      }
>>>
>>> +    /* those checks are going away next */
>>
>> This comment seems to imply a story documented in code. Rather just drop
>> it -- the patch description already points that out.
>>
>>> +    assert((block->target_start & ~target_page_mask) == 0);
>>> +    assert((block->target_end & ~target_page_mask) == 0);
>>> +    *pfnptr = dump_paddr_to_pfn(s, addr);
>>>      if (bufptr) {
>>>          *bufptr = buf;
>>>      }
>>
>>
>> Apart from the nits, LGTM.
> 
> We could also drop this patch, it helped me to rewrite the function next 
> mostly.

I think it's fine. Small logical changes are easier to review -- at
least for me.

-- 
Thanks,

David / dhildenb