Re: [libvirt] [PATCH] qga: ignore non present cpus when handling qmp_guest_get_vcpus()

+Drew On 08/30/18 14:08, Igor Mammedov wrote: > If VM has VCPUs plugged sparselly (for example a VM started with > 3 VCPUs (cpu0, cpu1 and cpu2) and then cpu1 was hotunplugged so > only cpu0 and cpu2 are present), QGA will rise a error > error: internal error: unable to execute QEMU agent command 'guest-get-vcpus': > open("/sys/devices/system/cpu/cpu1/"): No such file or directory > when > virsh vcpucount FOO --guest > is executed. > Fix it by ignoring non present CPUs when fetching CPUs status from sysfs. > > Signed-off-by: Igor Mammedov <imammedo(a)redhat.com&gt; > --- > qga/commands-posix.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/qga/commands-posix.c b/qga/commands-posix.c > index 37e8a2d..2929872 100644 > --- a/qga/commands-posix.c > +++ b/qga/commands-posix.c > @@ -2044,7 +2044,9 @@ static void transfer_vcpu(GuestLogicalProcessor *vcpu, bool sys2vcpu, > vcpu->logical_id); > dirfd = open(dirpath, O_RDONLY | O_DIRECTORY); > if (dirfd == -1) { > - error_setg_errno(errp, errno, "open(\"%s\")", dirpath); > + if (!(sys2vcpu && errno == ENOENT)) { > + error_setg_errno(errp, errno, "open(\"%s\")", dirpath); > + } > } else { > static const char fn[] = "online"; > int fd; > Originally these guest agent commands (both getting and setting) were meant to be used in the absence of real VCPU hot[un]plug, as a fallback / place-holder. If the latter (= real VCPU hot(un)plug) works, then these guest agent commands shouldn't be used at all.

Drew, do I remember correctly? ... The related RHBZ is <https://bugzilla.redhat.com/show_bug.cgi?id=924684>;. (It's a private one, and I'm not at liberty to open it up, so my apologies to non-RH folks.) Anyway, given that "set" should be a subset of the "get" return value (as documented in the command schema), if we fix up "get" to work with sparse topologies, then "set" should work at once. However... as far as I understand, this change will allow qmp_guest_get_vcpus() to produce a GuestLogicalProcessor object for the missing (hot-unplugged) VCPU, with the following contents: - @logical-id: populated by the loop, - @online: false (from g_malloc0()), - @can-offline: present (from the loop), and false (from g_malloc0()). The smaller problem with this might be that "online==false && can-offline==false" is nonsensical and has never been returned before. I don't know how higher level apps will react. The larger problem might be that a higher level app could simply copy this output structure into the next "set" call unchanged, and then that "set" call will fail.

I wonder if, instead of this patch, we should rework qmp_guest_get_vcpus(), to silently skip processors for which this dirpath ENOENT condition arises (i.e., return a shorter list of GuestLogicalProcessor objects).

But, again, I wouldn't mix this guest agent command with real VCPU hot(un)plug in the first place. The latter is much-much better, so if it's available, use that exclusively?

Thanks, Laszlo