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

[Janosch Frank]

On 7/13/22 17:35, Marc-André Lureau wrote:
> 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!

We have tests for s390 and I've prompted for tests with filtering so we 
can also cover that. Unfortunately s390 differs in the use of memory 
because we only have one large block which hid this error from me.


> > > > +        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?

Had a look, turns out I missed 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.

Maybe we should call this "dump_get_memblock_start_offset()" to make it 
clearer that we don't return block->target_start i.e. a start address 
but rather an offset that we tack on the host address to read the memory?

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