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> Thanks, -- Peter Xu
