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

[Michal Novotny]

On 06/26/2013 04:29 PM, Martin Kletzander wrote:
> 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 at redhat.com>
>>>> ---
>>>>  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
>

Feel free to push with changes you proposed, I need to take care of
other stuff ;-)

Thanks,
Michal

-- 
Michal Novotny <minovotn at redhat.com>, RHCE, Red Hat
Virtualization | libvirt-php bindings | php-virt-control.org