| Message ID | 20230517155219.10691-4-avihaih@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | migration: Add precopy initial data capability and VFIO precopy support | expand |
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.. PS: sorry to be late on replying to your email for previous version due to travelling last week, I'll reply to your series instead. Actually I was just writting up the reply to your previous version when receiving this one. :) Thanks,
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). > > PS: sorry to be late on replying to your email for previous version due to > travelling last week, I'll reply to your series instead. Actually I was > just writting up the reply to your previous version when receiving this > one. :) No worries, thanks :)
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> Thanks,
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!
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; } return true;