Re: [PATCH v2 3/7] migration: Enable precopy initial data capability

[Avihai Horon <avihaih@nvidia.com>]

On 18/05/2023 16:42, Peter Xu wrote:
> External email: Use caution opening links or attachments
>
>
> On Thu, May 18, 2023 at 10:26:04AM +0300, Avihai Horon wrote:
>> On 17/05/2023 19:07, Peter Xu wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> On Wed, May 17, 2023 at 06:52:15PM +0300, Avihai Horon wrote:
>>>> Now that precopy initial data logic has been implemented, enable the
>>>> capability.
>>>>
>>>> Signed-off-by: Avihai Horon <avihaih@nvidia.com>
>>>> Reviewed-by: Juan Quintela <quintela@redhat.com>
>>>> ---
>>>>    migration/options.c | 4 ----
>>>>    1 file changed, 4 deletions(-)
>>>>
>>>> diff --git a/migration/options.c b/migration/options.c
>>>> index 0a31921a7a..3449ce4f14 100644
>>>> --- a/migration/options.c
>>>> +++ b/migration/options.c
>>>> @@ -561,10 +561,6 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp)
>>>>                                 "capability 'return-path'");
>>>>                return false;
>>>>            }
>>>> -
>>>> -        /* Disable this capability until it's implemented */
>>>> -        error_setg(errp, "'precopy-initial-data' is not implemented yet");
>>>> -        return false;
>>>>        }
>>> I'm always confused why we need this and not having this squashed into
>>> patch 1 (or say, never have these lines).
>>>
>>> The only thing it matters is when someone backports patch 1 but not
>>> backport the rest of the patches.  But that's really, really weird already
>>> as a backporter doing that, and I doubt its happening.
>>>
>>> Neither should we merge patch 1 without merging follow up patches to
>>> master, as we should just always merge the whole feature or just keep
>>> reworking on the list.
>>>
>>> I'd like to know if I missed something else..
>> There are also git bisect considerations.
>> This practice is useful for git bisect for features that are enabled by
>> default, so you won't mistakenly run "half a feature" if you do bisect.
>> But here the capability must be manually enabled, so maybe it's not that
>> useful in this case.
>>
>> I like it for the sake of good order, so this capability can't be enabled
>> before it's fully implemented (even if it's unlikely that someone would do
>> that).
> Right.  I was kind of thinking someone bisecting such feature will always
> make sure to start from the last commit got merged, but I see your point as
> a general concept.
>
> One slightly better way to not add something and remove again is, we can
> introduce migrate_precopy_initial_data() in patch 2, returning constantly
> false, then we can put patch 1 (qapi interface) to be after current patch
> 2, where you allow migrate_precopy_initial_data() to start return true.  It
> saves a few lines to remove, and also one specific patch explicitly
> removing it.  But I think fundamentally it's similar indeed.
>
> In case you'd like to keep this as is, feel free to add:
>
> Reviewed-by: Peter Xu <peterx@redhat.com>

I think I will keep it as is, it seems more natural to me.
However, if someone insists then I don't mind changing it.

Thanks!