Re: [PATCH v4 02/17] dump: Introduce GuestPhysBlock offset and length filter functions

[Janis Schoetterl-Glausch]

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 <scgl@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 <frankja@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