Re: [PATCH v4 04/17] dump: Rework get_start_block

[Janis Schoetterl-Glausch]

On 7/26/22 11:22, Janosch Frank wrote:
> get_start_block() returns the start address of the first memory block
> or -1.
> 
> With the GuestPhysBlock iterator conversion we don't need to set the
> start address and can therefore remove that code. The only
> functionality left is the validation of the start block so it only
> makes sense to re-name the function to validate_start_block()
> 
> Signed-off-by: Janosch Frank <fran...@linux.ibm.com>

Reviewed-by: Janis Schoetterl-Glausch <s...@linux.ibm.com>

See suggenstions below.

> ---
>  dump/dump.c | 20 ++++++--------------
>  1 file changed, 6 insertions(+), 14 deletions(-)
> 
> diff --git a/dump/dump.c b/dump/dump.c
> index 35b9833a00..b59faf9941 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -1498,30 +1498,22 @@ static void create_kdump_vmcore(DumpState *s, Error 
> **errp)
>      }
>  }
>  
> -static ram_addr_t get_start_block(DumpState *s)
> +static int validate_start_block(DumpState *s)
>  {
>      GuestPhysBlock *block;
>  
>      if (!s->has_filter) {
> -        s->next_block = QTAILQ_FIRST(&s->guest_phys_blocks.head);
>          return 0;
>      }
>  
>      QTAILQ_FOREACH(block, &s->guest_phys_blocks.head, next) {
> +        /* This block is out of the range */
>          if (block->target_start >= s->begin + s->length ||
>              block->target_end <= s->begin) {
> -            /* This block is out of the range */
>              continue;
>          }
> -
> -        s->next_block = block;
> -        if (s->begin > block->target_start) {
> -            s->start = s->begin - block->target_start;
> -        } else {
> -            s->start = 0;
> -        }
> -        return s->start;
> -    }
> +        return 0;
> +   }
>  
>      return -1;
>  }

If you change the dump_get_memblock_* functions to take the DumpState, you 
could do

bool has_unfiltered_mem(DumpState *s)
{
    GuestPhysBlock *block;

    QTAILQ_FOREACH(block, &s->guest_phys_blocks.head, next) {
        if (dump_get_memblock_size(block, s) > 0) {
            return true;
        }
    }
    return false;
}

or you could do the same with the existing dump_get_memblock_size, add

    if (has_filter && !length) {
        error_setg(errp, QERR_INVALID_PARAMETER, "length");
        goto cleanup;
    }

to dump_init, encode has_filter in length as you're currently doing and get rid 
of s->has_filter entirely.

> @@ -1668,8 +1660,8 @@ static void dump_init(DumpState *s, int fd, bool 
> has_format,
>          goto cleanup;
>      }
>  
> -    s->start = get_start_block(s);
> -    if (s->start == -1) {
> +    /* Is the filter filtering everything? */
> +    if (validate_start_block(s) == -1) {
>          error_setg(errp, QERR_INVALID_PARAMETER, "begin");
>          goto cleanup;
>      }