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