On 11/16/21 18:29, Ben Gardon wrote:

quoted

TL;DR: this type of optional refactoring doesn't belong in a patch Cc'd for stable,
and my personal preference is to always declare variables at function scope (it's
not a hard rule though, Paolo has overruled me at least once:-)  ).

That makes sense. I don't have a preference either way. Paolo, if you
want the version without the refactor, the version I sent in the RFC
should be good. If the refactor is desired, I can separate it out into
another patch and send a v2 of this patch as a mini series, tagging
only the fix for stable.

It's really a damned-if-you-do/damned-if-you-don't situation.  And also 
keeping the patch as similar as possible in stable has the advantage 
that future backports have a slightly lower chance of breaking due to 
shadowed variables.

In the end I agree with both of you :) and in this case I tend to accept 
the patch as written.  So I queued it, though it probably will not be in 
the immediately next pull request.

My plan for the next couple days is to send a pull request and finally 
move the development tree to 5.16-rc1, so that I can push to kvm/next 
all the SVM, memslot and xarray stuff that's pending.  Then I'll go back 
to this one.

Paolo

I've generally preferred declaring variables at function scope too
since that seems like the overwhelming convention, but it's always
struck me as a bit of a waste to not make use of scoping rules more.
It does make it nice and clear how things should be laid out when
debugging the kernel with GDB or something though.

In any case, please let me know how you'd like the changes organized
and I can send up follow ups as needed, or we can just move forward
with the RFC version.