Re: [Patch v4 12/13] KVM: selftests: Make vCPU exit reason test assertion common.

[Vipin Sharma]

On Thu, Feb 2, 2023 at 10:51 AM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> On Thu, Feb 02, 2023, Vipin Sharma wrote:
> > On Wed, Feb 1, 2023 at 3:24 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
> > > I love the cleanup, but in the future, please don't squeeze KVM-wide changes in
> > > the middle of an otherwise arch-specific series unless it's absolutely necessary.
> > > I get why you added the macro before copy-pasting more code into a new test, but
> > > the unfortunate side effect is that complicates grabbing the entire series.
> > >
> >
> > Make sense. So what is preferable:
> > 1. Make the big cleanup identified during a series as the last patches
> > in that series?
> > 2. Have two series and big cleanups rebased on top of the initial series?
> >
> > Or, both 1 & 2 are acceptable depending on the cleanup?
>
>   3. Post the cleanup independently, but make a note so that maintainers know
>      that there may be conflicts and/or missed cleanup opportunities.
>
> #1 is rarely going to be the best option.  The big cleanup is going to necessitate
> Cc'ing a lot of people that don't care about the base arch-specific changes, so
> unless the base changes are one or two trivial patches, a lot of people end up
> having to wade through a lot of noise.  And aside from annoying people, that also
> makes it more likely that someone will overlook the cleanup.
>
> As for #2 vs. #3, #3 is probably a better option in most cases.  For broad cleanups,
> odds are very good that there will be other conflicts beyond just the changes _you_
> have in-flight.  E.g. in this case, any new tests and/or asserts that are in-flight,
> sitting in other trees, etc., will suffer the same fate.  I.e. whoever applies the
> cleanup is going to need to resolve conflicts and/or look for other cleanup
> opportunities anyways.  For a scenario like this, a way to make life easy for the
> maintainer applying the cleanup would be to provide a script, e.g. single grep
> command, to look for potential cleanup spots.  That communicates to the maintainer
> that there may be silent "conflicts" and makes it easier for them to resolve such
> conflicts.

This is a good idea, to provide a grep or at least provide hints on
how one has found places to edit. I will keep this in mind. Thanks

>
> Posting the cleanup separately means the two series/patches can proceed
> independently, e.g. respinning one doesn't screw up the other, maintainers can
> take the patches in whatever order they prefer, etc.
>
> There are undoubtedly exceptions, e.g. if the resulting conflicts are really nasty,
> but those should be few and far between.