Re: [libvirt] [PATCH v4] qemu: Implement CPUs check against machine type's cpu-max

On 06/26/2013 04:17 PM, Martin Kletzander wrote: > On 06/25/2013 05:44 PM, Michal Novotny wrote: >> Implement check whether (maximum) vCPUs doesn't exceed machine >> type's cpu-max settings. >> >> Differences between v3 and v4 (this one): >> - Rebased to latest libvirt version >> - Capability XML output extended by maxCpus field >> - Extended caps-qemu-kvm.xml test by maxCpus for one of test emulators >> >> On older versions of QEMU the check is disabled. >> >> Signed-off-by: Michal Novotny <minovotn(a)redhat.com&gt; >> --- >> docs/schemas/capability.rng | 5 ++++ >> src/conf/capabilities.c | 4 +++ >> src/conf/capabilities.h | 1 + >> src/qemu/qemu_capabilities.c | 41 +++++++++++++++++++++++++++- >> src/qemu/qemu_capabilities.h | 3 +- >> src/qemu/qemu_monitor.h | 1 + >> src/qemu/qemu_monitor_json.c | 6 ++++ >> src/qemu/qemu_process.c | 27 ++++++++++++++++++ >> tests/capabilityschemadata/caps-qemu-kvm.xml | 16 +++++------ >> 9 files changed, 94 insertions(+), 10 deletions(-) >> > [...] >> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c > [...] >> @@ -1789,9 +1801,11 @@ void virQEMUCapsDispose(void *obj) >> for (i = 0; i < qemuCaps->nmachineTypes; i++) { >> VIR_FREE(qemuCaps->machineTypes[i]); >> VIR_FREE(qemuCaps->machineAliases[i]); >> + qemuCaps->machineMaxCpus[i] = -1; > Pointless line. > >> } >> VIR_FREE(qemuCaps->machineTypes); >> VIR_FREE(qemuCaps->machineAliases); >> + VIR_FREE(qemuCaps->machineMaxCpus); >> >> for (i = 0; i < qemuCaps->ncpuDefinitions; i++) { >> VIR_FREE(qemuCaps->cpuDefinitions[i]); > [...] >> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h >> index 64a4b1d..7088747 100644 >> --- a/src/qemu/qemu_capabilities.h >> +++ b/src/qemu/qemu_capabilities.h >> @@ -234,7 +234,8 @@ size_t virQEMUCapsGetMachineTypes(virQEMUCapsPtr qemuCaps, >> char ***names); >> const char *virQEMUCapsGetCanonicalMachine(virQEMUCapsPtr qemuCaps, >> const char *name); >> - >> +int virQEMUCapsGetMachineMaxCpus(virQEMUCapsPtr qemuCaps, >> + const char *name); >> int virQEMUCapsGetMachineTypesCaps(virQEMUCapsPtr qemuCaps, >> size_t *nmachines, >> virCapsGuestMachinePtr **machines); >> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h >> index 3d9afa3..06ae4c5 100644 >> --- a/src/qemu/qemu_monitor.h >> +++ b/src/qemu/qemu_monitor.h >> @@ -654,6 +654,7 @@ struct _qemuMonitorMachineInfo { >> char *name; >> bool isDefault; >> char *alias; >> + int cpu_max; > This parameter should be unified to match the previous naming (maxCpus) > as cpu_max might be misunderstood as a maximum cpu number, not the > maximum number of cpus. > >> }; >> >> int qemuMonitorGetMachines(qemuMonitorPtr mon, > [...] >> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c >> index 5a0f18b..3146ce2 100644 >> --- a/src/qemu/qemu_process.c >> +++ b/src/qemu/qemu_process.c >> @@ -3330,6 +3330,30 @@ error: >> } >> >> >> +static bool >> +qemuValidateCpuMax(virDomainDefPtr def, virQEMUCapsPtr qemuCaps) >> +{ >> + int cpu_max; >> + >> + cpu_max = virQEMUCapsGetMachineMaxCpus(qemuCaps, def->os.machine); >> + if (!cpu_max) >> + return true; >> + >> + if (def->vcpus > cpu_max) { >> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, >> + "%s", _("CPUs greater than specified machine type limit")); >> + return false; >> + } >> + > This check is pointless since it's guaranteed that vcpus <= maxvcpus. > >> + if (def->maxvcpus > cpu_max) { >> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, >> + "%s", _("Maximum CPUs greater than specified machine type limit")); >> + return false; >> + } >> + >> + return true; >> +} >> + >> int qemuProcessStart(virConnectPtr conn, >> virQEMUDriverPtr driver, >> virDomainObjPtr vm, > Other that that the patch looks fine, but I'd wait with the push after > release. If nobody is against that (and against the changes I > proposed), I'll push this after the release. > > Martin Ok Martin, would you like me to do the changes you proposed or will you refresh the patch yourself and no need to submit v5 ?