Re: [PATCH v1 3/8] migration: Add vmstate part of migration stream

[Vladimir Sementsov-Ogievskiy]

23.03.2022 16:21, Nikita Lapshin wrote:
> On 3/23/22 15:49, Vladimir Sementsov-Ogievskiy wrote:
>
> > 23.03.2022 15:40, Vladimir Sementsov-Ogievskiy wrote:
> > > 23.03.2022 13:53, Nikita Lapshin wrote:
> > > > From: Nikita Lapshin<nikita.lapshin@virtuozzo.com>
> > > > Now we can disable and enable vmstate part by stream_content parameter.
> > > > Signed-off-by: Nikita Lapshin<nikita.lapshin@openvz.org>
> > > > ---
> > > >    migration/migration.c | 10 ++++++++--
> > > >    migration/savevm.c    | 13 +++++++++++++
> > > >    2 files changed, 21 insertions(+), 2 deletions(-)
> > > > diff --git a/migration/migration.c b/migration/migration.c
> > > > index 4adcc87d1d..bbf9b6aad1 100644
> > > > --- a/migration/migration.c
> > > > +++ b/migration/migration.c
> > > > @@ -1334,9 +1334,15 @@ void 
> > > > qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
> > > >        }
> > > >    }
> > > > -static bool check_stream_parts(strList *stream_content_list)
> > > > +static bool check_stream_parts(strList *stream_list)
> > > >    {
> > > > -    /* To be implemented in ext commits */
> > > > +    for (; stream_list; stream_list = stream_list->next) {
> > > > +        if (!strcmp(stream_list->value, "vmstate")) {
> > > > +            continue;
> > > > +        }
> > > > +
> > > > +        return false;
> > > > +    }
> > > >        return true;
> > > >    }
> > > When you move to enum for list elements in QAPI, this thing would be checked 
> > > automatically, you will not have to check it by hand.
> > > > diff --git a/migration/savevm.c b/migration/savevm.c
> > > > index c68f187ef7..8f35c0c81e 100644
> > > > --- a/migration/savevm.c
> > > > +++ b/migration/savevm.c
> > > > @@ -949,6 +949,19 @@ static bool should_skip(SaveStateEntry *se)
> > > >            return true;
> > > >        }
> > > > +    /*
> > > > +     * Assume that any SaveStateEntry with non-null vmsd is
> > > > +     * part of vmstate.
> > > > +     * Vmstate is included by default so firstly check if
> > > > +     * stream-content-list is enabled.
> > > > +     */
> > > > +
> > > > +    if (se->vmsd &&
> > > > +        migrate_get_current()->parameters.has_stream_content_list &&
> > > > +        !migrate_find_stream_content("vmstate")) {
> > > And in migrate_find_stream_content you do
> > >       if (!s->parameters.has_stream_content_list) {
> > >           return false;
> > >       }
> > > Seems better to do
> > >       if (!s->parameters.has_stream_content_list) {
> > >           return true;
> > >       }
> > > in migrate_find_stream_content(), and rename it to something like 
> > > should_migrate_content() or just migrate_content().
> > > Then here you don't need to check .has_stream_content_list.
> > Hmm, but that will work bad with dirty-bitmaps, as they are disabled by default.
> > So, this actually means that we have different default for different contents:
> > ram and and other device states are enabled by default, dirty-bitmaps are 
> > disabled by defaults. We can honestly realize these defaults in 
> > migrate_content().
>
> Yes, I agree that this logic looks quite strange. May be explicit checks can 
> help.


I think, checking for deprecated capabilites should be in same function 
migrate_content() too.

> > > > +        return true;
> > > > +    }
> > > > +
> > > >        return false;
> > > >    }


--
Best regards,
Vladimir