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;
+ if (block == NULL) {
+ *blockptr = block = QTAILQ_FIRST(&s->guest_phys_blocks.head);
+ 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);
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 */
+ 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;
}
--
2.37.2
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) {" ? :)
> + *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
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.
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
© 2016 - 2026 Red Hat, Inc.