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

[Janosch Frank]

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.


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?

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