On 26.08.22 11:56, Marc-André Lureau wrote: > Hi > > On Fri, Aug 26, 2022 at 1:45 PM David Hildenbrand <[email protected]> wrote: >> >> On 25.08.22 15:21, [email protected] wrote: >>> From: Marc-André Lureau <[email protected]> >>> >>> 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 <[email protected]> >>> --- >>> 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
