On Fri, Dec 15, 2017 at 14:11:41 +0100, Andrea Bolognani wrote:
On Fri, 2017-12-15 at 13:18 +0100, Peter Krempa wrote:
On Thu, Dec 14, 2017 at 17:29:50 +0100, Andrea Bolognani wrote:
While at the moment we're only performing a single check that is connected to vCPU hotplugging, we're going to introduce a second one soon. Move the topology check underneath the capability check to make that easier; as a bonus, doing so allows us to reduce the scope of the 'topologycpus' variable.
You know that generally we prefer variables declared at the top scope?
Is that so? I've seen several instances of the opposite, and it makes to me not to pollute the function scope with one-use variables when there are usually enough variables that actually need to be accessed throughout the function. But I can leave the declaration where it is if you like it better that way.
Fair enough. We only state that we should declare it at the beginning of a scope, so while generally most are declared at function scoep, this does not violate the contributor guidelines.
Also you change the version of qemu in the comment without explanation here. Rather than bragging how you reduced scope, please document that change.
Right, I meant to include the explanation but forgot :)
It's very simple, anyway: the version number was wrong, since QEMU introduced query-hotpluggable-cpus in 2.7 rather than 2.5.
While this is true for the presence of query-hotpluggable-cpus. The paragraph is specifically saying that QEMU rejects such configurations starting from 2.5., but the code checks them only since query-hotpluggable-cpus (which was introduced in 2.7). Your modification is thus incorrect. You can add statement that query-hotpluggable-cpus was added in 2.7. but mangling it as you did is not correct in my opinion.