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

[Vladimir Sementsov-Ogievskiy]

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().

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


--
Best regards,
Vladimir