Re: [PATCH v10 2/10] libs/guest: move batch_pfns into a separate structure

On 13/08/2026 12:27 pm, Jan Beulich wrote:
> On 13.08.2026 13:08, Andrew Cooper wrote:
>> On 10/08/2026 11:30 am, Frediano Ziglio wrote:
>>> Preparation for a followup patch "libs/guest: allocate various migration
>>> arrays just once".
>>>
>>> Signed-off-by: Frediano Ziglio <[email protected]>
>>> Reviewed-by: Anthony PERARD <[email protected]>
>> Coverity thinks this change has memory corruption.  I have to admit that
>> I'm not completely sure why it's noticed now; possibly because now it
>> can see the size of batch_pfns[] where previously it couldn't
> I had looked into that too, and I'm puzzled that ...
>
>> ** CID 1700057:       Memory - corruptions  (OVERRUN)
>> /tools/libs/guest/xg_sr_save.c: 284           in add_to_batch()
>> _____________________________________________________________________________________________
>> *** CID 1700057:         Memory - corruptions  (OVERRUN)
>> /tools/libs/guest/xg_sr_save.c: 284             in add_to_batch()
>> 278         int rc = 0;
>> 279     
>> 280         if ( ctx->save.nr_batch_pfns == MAX_BATCH_SIZE )
>> 281             rc = flush_batch(ctx);
> ... the tool can't spot that flush_batch() resets ctx->save.nr_batch_pfns
> to 0 in the success case. And ...
>
>> 283         if ( rc == 0 )
> ... only the success case is what matters.

Hmm.  Both flush_batch() and write_batch() are static, so fully visible
to Coverity.

I guess this means that Coverity failed to figure out the properties of
write_batch(); it is a complicated function, even if it has become less
complicated recently.

I'm still advocating to remove the Valgrind logic rather than extend it
in patch 3, and that will remove flush_batch() which might simplify things.

~Andrew