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

[Vladimir Sementsov-Ogievskiy <v.sementsov-og@mail.ru>]

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.

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


-- 
Best regards,
Vladimir