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 <avih...@nvidia.com>
Reviewed-by: Juan Quintela <quint...@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 <pet...@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!
