On 7/26/22 11:22, Janosch Frank wrote: > The iteration over the memblocks is hard to understand so it's about > time to clean it up. Instead of manually grabbing the next memblock we > can use QTAILQ_FOREACH to iterate over all memblocks.
This got out of sync with the patch, didn't it? With that addressed: Reviewed-by: Janis Schoetterl-Glausch <s...@linux.ibm.com> See minor stuff below. > > Additionally we move the calculation of the offset and length out by > using the dump_get_memblock_*() functions. > > Signed-off-by: Janosch Frank <fran...@linux.ibm.com> > --- > dump/dump.c | 37 +++++++++++++++++++++++++++++++++++++ > include/sysemu/dump.h | 5 +++++ > 2 files changed, 42 insertions(+) > > diff --git a/dump/dump.c b/dump/dump.c > index 0ed7cf9c7b..0fd7c76c1e 100644 > --- a/dump/dump.c > +++ b/dump/dump.c > @@ -591,6 +591,43 @@ static void dump_begin(DumpState *s, Error **errp) > write_elf_notes(s, errp); > } > > +int64_t dump_get_memblock_size(GuestPhysBlock *block, int64_t > filter_area_start, > + int64_t filter_area_length)> +{ > + int64_t size, left, right; I assume the int64_t everywhere are because DumpState.begin and .length are int64_t, which is itself because the numbers are coming from a command? There isn't any reason to have negative numbers for that command, is there? Since the block->target_* are unsigned we'd get problems with negative numbers. Ideally the the values should be checked up the stack and unsigned used in this function, IMO, but it's not a big deal either. > + > + /* No filter, return full size */ > + if (!filter_area_length) { > + return block->target_end - block->target_start; > + } > + > + /* calculate the overlapped region. */ > + left = MAX(filter_area_start, block->target_start); > + right = MIN(filter_area_start + filter_area_length, block->target_end); > + size = right - left; > + size = size > 0 ? size : 0; > + > + return size; > +} > + > +int64_t dump_get_memblock_start(GuestPhysBlock *block, int64_t > filter_area_start, > + int64_t filter_area_length) > +{ > + if (filter_area_length) { > + /* return -1 if the block is not within filter area */ > + 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 0; > +} > + > static int get_next_block(DumpState *s, GuestPhysBlock *block) > { > while (1) { > diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h > index ffc2ea1072..6ce3c24197 100644 > --- a/include/sysemu/dump.h > +++ b/include/sysemu/dump.h > @@ -203,4 +203,9 @@ typedef struct DumpState { > uint16_t cpu_to_dump16(DumpState *s, uint16_t val); > uint32_t cpu_to_dump32(DumpState *s, uint32_t val); > uint64_t cpu_to_dump64(DumpState *s, uint64_t val); > + > +int64_t dump_get_memblock_size(GuestPhysBlock *block, int64_t > filter_area_start, > + int64_t filter_area_length); > +int64_t dump_get_memblock_start(GuestPhysBlock *block, int64_t > filter_area_start, > + int64_t filter_area_length); I don't love the names of the functions, maybe dump_filtered_block_size, dump_filtered_block_offset? > #endif