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

[Frediano Ziglio <freddy77@xxxxxxxxx>]

On Thu, 13 Aug 2026 at 12:48, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
>
> 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 <frediano.ziglio@xxxxxxxxxx>
> >>> Reviewed-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> >> 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

Hi,
   as said by Jan this is a false positive from Coverity.

It's not guaranteed that Coverity will detect that it's not an issue
if either valgrind macro is not called and/or flush_batch calling
write_batch, I would either test the change or simply add an explicit
Coverity comment.
If we decide to remove support for valgrind then what about the
valgrind patch on the series (4/10) ?

Frediano