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

[Janosch Frank <frankja@linux.ibm.com>]

On 7/26/22 13:35, Marc-André Lureau wrote:
> On Tue, Jul 26, 2022 at 1:23 PM Janosch Frank <frankja@linux.ibm.com> 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.
>>
>> 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;
>> +
>> +    /* 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);
> 
> The functions don't need to be exported. You probably need to
> introduce them back with their usage, to avoid some compiler warning.

Right, I'll add them in the last s390 dump patch and make them static

> If you can't split the introduction and related refactoring, then
> let's have a single patch.

So squashing this with the next one but leave the other refactoring 
patches (dump_calculate_size() and get_start_block()) as they are?

> 
> Thanks
> 
>>   #endif
>> --
>> 2.34.1
>>
> 
>