Hi

On Thu, Jul 14, 2022 at 1:46 PM Janosch Frank <fran...@linux.ibm.com> wrote:

> On 7/13/22 17:35, Marc-André Lureau wrote:
> > Hi
> >
> > On Wed, Jul 13, 2022 at 7:30 PM Janosch Frank <fran...@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 <fran...@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 <fran...@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?
>
>
Not a big difference to me. You would need to adjust write_memory() "start"
argument name as well then.


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

-- 
Marc-André Lureau

Reply via email to