Re: [libvirt] [PATCH 1/2] qemu: Invert condition nesting in qemuDomainDefValidate()
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.
Attachments:
- signature.asc (application/pgp-signature — 833 bytes)