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

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

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