Re: [PATCH v2 01/11] dump: Cleanup memblock usage

[Marc-André Lureau]

Hi

On Wed, Jul 13, 2022 at 7:30 PM Janosch Frank <frankja@linux.ibm.com> wrote:
>
> On 7/13/22 17:09, Marc-André Lureau wrote:
> > Hi
> >
> > On Wed, Jul 13, 2022 at 5:07 PM Janosch Frank <frankja@linux.ibm.com> wrote:
> >>
> >> The iteration over the memblocks is hard to understand so it's about
> >> time to clean it up.
> >>
> >> struct DumpState's next_block and start members can and should be
> >> local variables within the iterator.
> >>
> >> Instead of manually grabbing the next memblock we can use
> >> QTAILQ_FOREACH to iterate over all memblocks.
> >>
> >> The begin and length fields in the DumpState have been left untouched
> >> since the qmp arguments share their names.
> >>
> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> >
> > After this patch:
> > ./qemu-system-x86_64 -monitor stdio -S
> > (qemu) dump-guest-memory foo
> > Error: dump: failed to save memory: Bad address
>
> If you have more ways to check for dump errors then please send them to
> me. I'm aware that this might not have been a 100% conversion and I'm a
> bit terrified about the fact that this will affect all architectures.

Same feeling here. Maybe it's about time to write real dump tests!

>
>
> Anyway, I'll have a look.
>
> [...]
>
> >> +static inline int64_t dump_get_memblock_start(GuestPhysBlock *block, 
> >> int64_t filter_area_start,
> >> +                                  int64_t filter_area_length)
> >> +{
> >> +    if (filter_area_length) {
> >> +        /*
> >> +         * Check if block is within guest memory dump area. If not
> >> +         * go to next one.
> >> +         */
> >
> > Or rather "return -1 if the block is not within filter area"
>
> Sure
>
> >
> >> +        if (block->target_start >= filter_area_start + filter_area_length 
> >> ||
> >> +            block->target_end <= filter_area_start) {
> >> +            return -1;
> >> +        }
> >> +        if (filter_area_start > block->target_start) {
> >> +            return filter_area_start - block->target_start;
> >> +        }
> >> +    }
> >> +    return block->target_start;
> >
> > This used to be 0. Changing that, I think the patch looks good.
> > Although it could perhaps be splitted to introduce the two functions.
>
> Yes but the 0 was used to indicate that we would have needed continue
> iterating and the iteration is done via other means in this patch.
>
> Or am I missing something?

Well, you changed the way the loop used to work. it used to return 1/0
to indicate stop/continue and rely on s->start / s->next_block. Now
you return memblock_start.

>
> >
> >> +}
> >>   #endif
> >> --
> >> 2.34.1
> >>
> >
> >
>