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) {" ? :)
> + *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.
--
Thanks,
David / dhildenb
