[libvirt] [PATCH 0/5] qemu: fix individual vcpu hotplug code
Resolve a few corner cases which would create invalid configurations or produce bad error messages. Peter Krempa (5): qemu: hotplug: Iterate over vcpu 0 in individual vcpu hotplug code qemu: hotplug: Fix formatting strings in qemuDomainFilterHotplugVcpuEntities qemu: hotplug: Clear vcpu ordering for coldplug of vcpus qemu: hotplug: Add validation for coldplug of individual vcpus qemu: hotplug: Validate that vcpu-hotplug does not break config src/qemu/qemu_hotplug.c | 73 +++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 65 insertions(+), 8 deletions(-) -- 2.12.2
Buggy condition meant that vcpu0 would not be iterated in the checks. Since it's not hotpluggable anyways we would not be able to break the configuration of a live VM. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1437013 --- src/qemu/qemu_hotplug.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index de561da58..b5b520d8c 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -5784,7 +5784,7 @@ qemuDomainSetVcpuConfig(virDomainDefPtr def, def->individualvcpus = true; - while ((next = virBitmapNextSetBit(map, next)) > 0) { + while ((next = virBitmapNextSetBit(map, next)) >= 0) { if (!(vcpu = virDomainDefGetVcpu(def, next))) continue; @@ -5817,7 +5817,7 @@ qemuDomainFilterHotplugVcpuEntities(virDomainDefPtr def, return NULL; /* make sure that all selected vcpus are in the correct state */ - while ((next = virBitmapNextSetBit(map, next)) > 0) { + while ((next = virBitmapNextSetBit(map, next)) >= 0) { if (!(vcpu = virDomainDefGetVcpu(def, next))) continue; @@ -5837,7 +5837,7 @@ qemuDomainFilterHotplugVcpuEntities(virDomainDefPtr def, /* Make sure that all vCPUs belonging to a single hotpluggable entity were * selected and then de-select any sub-threads of it. */ next = -1; - while ((next = virBitmapNextSetBit(map, next)) > 0) { + while ((next = virBitmapNextSetBit(map, next)) >= 0) { if (!(vcpu = virDomainDefGetVcpu(def, next))) continue; -- 2.12.2
'next' is declared as 'ssize_t' so use '%zd' --- src/qemu/qemu_hotplug.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index b5b520d8c..48de6b815 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -5823,13 +5823,13 @@ qemuDomainFilterHotplugVcpuEntities(virDomainDefPtr def, if (vcpu->online == state) { virReportError(VIR_ERR_INVALID_ARG, - _("vcpu '%zu' is already in requested state"), next); + _("vcpu '%zd' is already in requested state"), next); goto cleanup; } if (vcpu->online && !vcpu->hotpluggable) { virReportError(VIR_ERR_INVALID_ARG, - _("vcpu '%zu' can't be hotunplugged"), next); + _("vcpu '%zd' can't be hotunplugged"), next); goto cleanup; } } @@ -5845,7 +5845,7 @@ qemuDomainFilterHotplugVcpuEntities(virDomainDefPtr def, if (vcpupriv->vcpus == 0) { virReportError(VIR_ERR_INVALID_ARG, - _("vcpu '%zu' belongs to a larger hotpluggable entity, " + _("vcpu '%zd' belongs to a larger hotpluggable entity, " "but siblings were not selected"), next); goto cleanup; } @@ -5854,7 +5854,7 @@ qemuDomainFilterHotplugVcpuEntities(virDomainDefPtr def, if (!virBitmapIsBitSet(map, i)) { virReportError(VIR_ERR_INVALID_ARG, _("vcpu '%zu' was not selected but it belongs to " - "hotpluggable entity '%zu-%zu' which was " + "hotpluggable entity '%zd-%zd' which was " "partially selected"), i, next, next + vcpupriv->vcpus - 1); goto cleanup; -- 2.12.2
Vcpu order is required to stay sequential. Clear the order on cpu coldplug to avoid issues with removing vcpus out of sequence. --- src/qemu/qemu_hotplug.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 48de6b815..5488b1dd4 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -5784,13 +5784,15 @@ qemuDomainSetVcpuConfig(virDomainDefPtr def, def->individualvcpus = true; + /* ordering information may become invalid, thus clear it */ + virDomainDefVcpuOrderClear(def); + while ((next = virBitmapNextSetBit(map, next)) >= 0) { if (!(vcpu = virDomainDefGetVcpu(def, next))) continue; vcpu->online = state; vcpu->hotpluggable = VIR_TRISTATE_BOOL_YES; - vcpu->order = 0; } } -- 2.12.2
Validate that users don't try to disable vcpu 0 and reject attempt to modify a vcpu to the state it is currently in. --- src/qemu/qemu_hotplug.c | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 5488b1dd4..18a8df33a 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -5875,6 +5875,37 @@ qemuDomainFilterHotplugVcpuEntities(virDomainDefPtr def, } +static int +qemuDomainVcpuValidateConfig(virDomainDefPtr def, + virBitmapPtr map, + bool state) +{ + virDomainVcpuDefPtr vcpu; + ssize_t next = -1; + + /* vcpu 0 can't be disabled */ + if (!state && virBitmapIsBitSet(map, 0)) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("vCPU '0' must be enabled")); + return -1; + } + + /* make sure that all selected vcpus are in the correct state */ + while ((next = virBitmapNextSetBit(map, next)) >= 0) { + if (!(vcpu = virDomainDefGetVcpu(def, next))) + continue; + + if (vcpu->online == state) { + virReportError(VIR_ERR_INVALID_ARG, + _("vcpu '%zd' is already in requested state"), next); + return -1; + } + } + + return 0; +} + + int qemuDomainSetVcpuInternal(virQEMUDriverPtr driver, virDomainObjPtr vm, @@ -5909,6 +5940,11 @@ qemuDomainSetVcpuInternal(virQEMUDriverPtr driver, } } + if (persistentDef) { + if (qemuDomainVcpuValidateConfig(persistentDef, map, state) < 0) + goto cleanup; + } + if (livevcpus && qemuDomainSetVcpusLive(driver, cfg, vm, livevcpus, state) < 0) goto cleanup; -- 2.12.2
On 03/31/2017 07:52 AM, Peter Krempa wrote:
Validate that users don't try to disable vcpu 0 and reject attempt to modify a vcpu to the state it is currently in. --- src/qemu/qemu_hotplug.c | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 5488b1dd4..18a8df33a 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -5875,6 +5875,37 @@ qemuDomainFilterHotplugVcpuEntities(virDomainDefPtr def, }
+static int +qemuDomainVcpuValidateConfig(virDomainDefPtr def, + virBitmapPtr map, + bool state) +{ + virDomainVcpuDefPtr vcpu; + ssize_t next = -1; + + /* vcpu 0 can't be disabled */ + if (!state && virBitmapIsBitSet(map, 0)) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("vCPU '0' must be enabled")); + return -1; + } + + /* make sure that all selected vcpus are in the correct state */ + while ((next = virBitmapNextSetBit(map, next)) >= 0) { + if (!(vcpu = virDomainDefGetVcpu(def, next))) + continue; + + if (vcpu->online == state) { + virReportError(VIR_ERR_INVALID_ARG, + _("vcpu '%zd' is already in requested state"), next); + return -1; + }
Does this really matter for this path? (config file). Wouldn't they just be changing to what they already have and is that really a big deal. IDC either way, but since there was no bz attached to this patch, I was just curious why the check. John
+ } + + return 0; +} + + int qemuDomainSetVcpuInternal(virQEMUDriverPtr driver, virDomainObjPtr vm, @@ -5909,6 +5940,11 @@ qemuDomainSetVcpuInternal(virQEMUDriverPtr driver, } }
+ if (persistentDef) { + if (qemuDomainVcpuValidateConfig(persistentDef, map, state) < 0) + goto cleanup; + } + if (livevcpus && qemuDomainSetVcpusLive(driver, cfg, vm, livevcpus, state) < 0) goto cleanup;
On Mon, Apr 03, 2017 at 18:24:59 -0400, John Ferlan wrote:
On 03/31/2017 07:52 AM, Peter Krempa wrote:
Validate that users don't try to disable vcpu 0 and reject attempt to modify a vcpu to the state it is currently in. --- src/qemu/qemu_hotplug.c | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 5488b1dd4..18a8df33a 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -5875,6 +5875,37 @@ qemuDomainFilterHotplugVcpuEntities(virDomainDefPtr def, }
+static int +qemuDomainVcpuValidateConfig(virDomainDefPtr def, + virBitmapPtr map, + bool state) +{ + virDomainVcpuDefPtr vcpu; + ssize_t next = -1; + + /* vcpu 0 can't be disabled */ + if (!state && virBitmapIsBitSet(map, 0)) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("vCPU '0' must be enabled")); + return -1; + } + + /* make sure that all selected vcpus are in the correct state */ + while ((next = virBitmapNextSetBit(map, next)) >= 0) { + if (!(vcpu = virDomainDefGetVcpu(def, next))) + continue; + + if (vcpu->online == state) { + virReportError(VIR_ERR_INVALID_ARG, + _("vcpu '%zd' is already in requested state"), next); + return -1; + }
Does this really matter for this path? (config file). Wouldn't they just be changing to what they already have and is that really a big deal.
I used the same check as in the online path, but you are right, it does not make much sense. I'll drop this check.
IDC either way, but since there was no bz attached to this patch, I was just curious why the check.
Bugs are there even when nobody reports them :)
Make sure that non-hotpluggable vcpus stay clustered at the beginning after modifying persistent definition. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1437010 --- src/qemu/qemu_hotplug.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 18a8df33a..d5aeb6bc0 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -5881,6 +5881,7 @@ qemuDomainVcpuValidateConfig(virDomainDefPtr def, bool state) { virDomainVcpuDefPtr vcpu; + size_t maxvcpus = virDomainDefGetVcpusMax(def); ssize_t next = -1; /* vcpu 0 can't be disabled */ @@ -5890,7 +5891,25 @@ qemuDomainVcpuValidateConfig(virDomainDefPtr def, return -1; } + /* non-hotpluggable vcpus need to stay clustered starting from vcpu 0 */ + for (next = virBitmapNextSetBit(map, -1) + 1; next < maxvcpus; next++) { + if (!(vcpu = virDomainDefGetVcpu(def, next))) + continue; + + /* skip vcpus being modified */ + if (virBitmapIsBitSet(map, next)) + continue; + + if (vcpu->online && vcpu->hotpluggable == VIR_TRISTATE_BOOL_NO) { + virReportError(VIR_ERR_INVALID_ARG, + _("vcpu '%zd' can't be modified as it is followed " + "by non-hotpluggable online vcpus"), next); + return -1; + } + } + /* make sure that all selected vcpus are in the correct state */ + next = -1; while ((next = virBitmapNextSetBit(map, next)) >= 0) { if (!(vcpu = virDomainDefGetVcpu(def, next))) continue; -- 2.12.2
On 03/31/2017 07:52 AM, Peter Krempa wrote:
Resolve a few corner cases which would create invalid configurations or produce bad error messages.
Peter Krempa (5): qemu: hotplug: Iterate over vcpu 0 in individual vcpu hotplug code qemu: hotplug: Fix formatting strings in qemuDomainFilterHotplugVcpuEntities qemu: hotplug: Clear vcpu ordering for coldplug of vcpus qemu: hotplug: Add validation for coldplug of individual vcpus qemu: hotplug: Validate that vcpu-hotplug does not break config
src/qemu/qemu_hotplug.c | 73 +++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 65 insertions(+), 8 deletions(-)
ACK series (left a note on patch 4) John
participants (2)
-
John Ferlan -
Peter Krempa