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

On 06/26/2013 04:20 PM, Michal Novotny wrote: > 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 ? > If you'll make it with the v5 before the release, the added value will be that everyone will be offered the look to how the final patch looks like, but if you won't I'll push this one with the changes I proposed. Martin