Re: [PULL 21/40] migration: per-mode blockers

[Peter Maydell]

On Thu, 9 Nov 2023 at 17:24, Steven Sistare <steven.sistare@oracle.com> wrote:
>
> On 11/9/2023 12:10 PM, Peter Maydell wrote:
> > On Thu, 2 Nov 2023 at 11:43, Juan Quintela <quintela@redhat.com> wrote:
> >>
> >> From: Steve Sistare <steven.sistare@oracle.com>
> >>
> >> Extend the blocker interface so that a blocker can be registered for
> >> one or more migration modes.  The existing interfaces register a
> >> blocker for all modes, and the new interfaces take a varargs list
> >> of modes.
> >>
> >> Internally, maintain a separate blocker list per mode.  The same Error
> >> object may be added to multiple lists.  When a block is deleted, it is
> >> removed from every list, and the Error is freed.
> >>
> >> No functional change until a new mode is added.
> >
> > Hi; Coverity worries about this code:
> >
> >> -static GSList *migration_blockers;
> >> +static GSList *migration_blockers[MIG_MODE__MAX];
> >>
> >>  static bool migration_object_check(MigrationState *ms, Error **errp);
> >>  static int migration_maybe_pause(MigrationState *s,
> >> @@ -1043,7 +1043,7 @@ static void fill_source_migration_info(MigrationInfo 
> >> *info)
> >>  {
> >>      MigrationState *s = migrate_get_current();
> >>      int state = qatomic_read(&s->state);
> >> -    GSList *cur_blocker = migration_blockers;
> >> +    GSList *cur_blocker = migration_blockers[migrate_mode()];
> >
> > because it thinks that migrate_mode() might return a value that's
> > too big for the migration_blockers[] array. (CID 1523829, 1523830.)
> >
> > I think Coverity complains mostly because it doesn't understand
> > that the MIG_MODE__MAX in the enum is not a valid enum value
> > that a function returning a MigMode might return. But we can
> > help it out by assert()ing in migrate_mode() that the value
> > we're about to return is definitely a valid mode.
>
> Thanks Peter, I will submit a fix with suggested-by, unless you want to.
> I'll look at all uses of migration_blocker[].
> Would coverity be happier if I also increase the size of the array?

Increasing the array size would also placate Coverity, but I think
in terms of actual bug possibilities the assert() is probably better,
as it would also catch the case of a garbage out-of-range value
getting written into the struct somehow.

thanks
-- PMM