Re: [libvirt] [PATCH 3/4] qemu: monitor: Add vcpu state information to monitor data

On Wed, Sep 14, 2016 at 10:26:51 +0800, Dou Liyang wrote: > Hi Peter, > > At 09/14/2016 12:27 AM, Peter Krempa wrote: >> Return whether a vcpu entry is hotpluggable or online so that upper >> layers don't have to infer the information from other data. >> >> Advantage is that this code can be tested by unit tests. >> --- >> src/qemu/qemu_monitor.c | 11 +++++ >> src/qemu/qemu_monitor.h | 4 ++ >> .../qemumonitorjson-cpuinfo-ppc64-basic.data | 48 ++++++++++++++++++++++ >> .../qemumonitorjson-cpuinfo-ppc64-hotplug-1.data | 48 ++++++++++++++++++++++ >> .../qemumonitorjson-cpuinfo-ppc64-hotplug-2.data | 48 ++++++++++++++++++++++ >> .../qemumonitorjson-cpuinfo-ppc64-hotplug-4.data | 48 ++++++++++++++++++++++ >> .../qemumonitorjson-cpuinfo-ppc64-no-threads.data | 32 +++++++++++++++ >> ...emumonitorjson-cpuinfo-x86-basic-pluggable.data | 16 ++++++++ >> .../qemumonitorjson-cpuinfo-x86-full.data | 22 ++++++++++ >> tests/qemumonitorjsontest.c | 3 ++ >> 10 files changed, 280 insertions(+) >> >> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c >> index 4489997..e700b15 100644 >> --- a/src/qemu/qemu_monitor.c >> +++ b/src/qemu/qemu_monitor.c >> @@ -1773,6 +1773,7 @@ qemuMonitorGetCPUInfoHotplug(struct qemuMonitorQueryHotpluggableCpusEntry *hotpl >> int order = 1; >> size_t totalvcpus = 0; >> size_t mastervcpu; /* this iterator is used for iterating hotpluggable entities */ >> + size_t slavevcpu; /* this corresponds to subentries of a hotpluggable entry */ >> size_t anyvcpu; /* this iterator is used for any vcpu entry in the result */ >> size_t i; >> size_t j; >> @@ -1816,6 +1817,10 @@ qemuMonitorGetCPUInfoHotplug(struct qemuMonitorQueryHotpluggableCpusEntry *hotpl >> * logical vcpus in the guest */ >> mastervcpu = 0; >> for (i = 0; i < nhotplugvcpus; i++) { >> + vcpus[mastervcpu].online = !!hotplugvcpus[i].qom_path; >> + vcpus[mastervcpu].hotpluggable = !!hotplugvcpus[i].alias; >> + if (!vcpus[mastervcpu].online) >> + vcpus[mastervcpu].hotpluggable = true; > > I think if the vcpu don't have an alias, we mark it as non-hotpluggable. > so, the code above may not fit well. The hotplugvcpus.alias field is NULL in the following two cases: 1) vcpu is online but was selected as non-hotpluggable 2) the vcpu is offline

The hotpluggable field in libvirt is covering both the hotplug and hotunplug case, thus a vcpu shall be marked as hotpluggable if: 1) it's offline 2) has the correct alias. > I guess you may mean that: > > vcpus[mastervcpu].online = !!hotplugvcpus[i].qom_path; > vcpus[mastervcpu].hotpluggable = false; > if (!vcpus[mastervcpu].online && (!!hotplugvcpus[i].alias)) > vcpus[mastervcpu].hotpluggable = true; This is wrong as online vcpus would not be marked as hotpluggable (which by the way in the libvirt context also means that it can be unplugged). > > OR > > vcpus[mastervcpu].online = !!hotplugvcpus[i].qom_path; > > if (vcpus[mastervcpu].online) > vcpus[mastervcpu].hotpluggable = false; This is wrong in the same regards. > else > vcpus[mastervcpu].hotpluggable = !!hotplugvcpus[i].alias; Also offline vcpu won't ever have an alias, since it's offline. Peter