On 11/16/21 18:29, Ben Gardon wrote:
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.