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?
Also you change the version of qemu in the comment without explanation
here. Rather than bragging how you reduced scope, please document that
change.
>
> Signed-off-by: Andrea Bolognani <abologna@xxxxxxxxxx>
> ---
> src/qemu/qemu_domain.c | 15 +++++++++------
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 74b82450b..e93333669 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -3299,7 +3299,6 @@ qemuDomainDefValidate(const virDomainDef *def,
> {
> virQEMUDriverPtr driver = opaque;
> virQEMUCapsPtr qemuCaps = NULL;
> - unsigned int topologycpus;
> int ret = -1;
>
> if (!(qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache,
> @@ -3359,11 +3358,15 @@ qemuDomainDefValidate(const virDomainDef *def,
> }
> }
>
> - /* qemu as of 2.5.0 rejects SMP topologies that don't match the cpu count */
> - if (virDomainDefGetVcpusTopology(def, &topologycpus) == 0 &&
> - topologycpus != virDomainDefGetVcpusMax(def)) {
> - /* presence of query-hotpluggable-cpus should be a good enough witness */
> - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_HOTPLUGGABLE_CPUS)) {
> + /* QEMU 2.7 (detected via the availability of query-hotpluggable-cpus)
> + * enforces stricter rules than previous versions when it comes to guest
> + * CPU topology. Verify known constraints are respected */
> + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_HOTPLUGGABLE_CPUS)) {
> + unsigned int topologycpus;
> +
> + /* Max vCPU count and overall vCPU topology must agree */
> + if (virDomainDefGetVcpusTopology(def, &topologycpus) == 0 &&
> + topologycpus != virDomainDefGetVcpusMax(def)) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("CPU topology doesn't match maximum vcpu count"));
> goto cleanup;
The code looks okay, but I want to see a justification on the changed
version.
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
