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

[Marc-André Lureau]

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.