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

On 08/31/18 10:00, Igor Mammedov wrote: > On Thu, 30 Aug 2018 17:51:13 +0200 > Laszlo Ersek <lersek(a)redhat.com&gt; wrote: > >> +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. > Technically there isn't reasons for "get" not to work in sparse usecase > hence the patch. > >> 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.

> Libvirt it seems that survives such outrageous output

>> 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? > Agreed, > > Maybe we can block invalid usecase on libvirt side with a more clear > error message as libvirt sort of knows that sparse cpus are supported. OK -- I see libvir-list is CC'd, so let's hear what they prefer :)