[Qemu-devel] [PATCH] qga: ignore non present cpus when handling qmp_guest_get_vcpus()

[Qemu-devel] [PATCH] qga: ignore non present cpus when handling qmp_guest_get_vcpus()

[Igor Mammedov]

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@redhat.com>
---
 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;
-- 
2.7.4

Re: [Qemu-devel] [PATCH] qga: ignore non present cpus when handling qmp_guest_get_vcpus()

[Laszlo Ersek]

+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@redhat.com>
> ---
>  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

Re: [Qemu-devel] [PATCH] qga: ignore non present cpus when handling qmp_guest_get_vcpus()

[Igor Mammedov]

On Thu, 30 Aug 2018 17:51:13 +0200
Laszlo Ersek <lersek@redhat.com> 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@redhat.com>
> > ---
> >  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.

> 
> Thanks,
> Laszlo

Re: [Qemu-devel] [PATCH] qga: ignore non present cpus when handling qmp_guest_get_vcpus()

[Laszlo Ersek]

On 08/31/18 10:00, Igor Mammedov wrote:
> On Thu, 30 Aug 2018 17:51:13 +0200
> Laszlo Ersek <lersek@redhat.com> 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@redhat.com>
>>> ---
>>>  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 :)

Thanks
Laszlo

Re: [Qemu-devel] [PATCH] qga: ignore non present cpus when handling qmp_guest_get_vcpus()

[Andrew Jones]

On Thu, Aug 30, 2018 at 05:51:13PM +0200, Laszlo Ersek wrote:
> 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?

Yup. This guest agent functionality was merely a stopgap to support "cpu
hotplug", before real cpu hotplug could be supported.

> 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).

That sounds good to me.

Thanks,
drew

Re: [Qemu-devel] [PATCH] qga: ignore non present cpus when handling qmp_guest_get_vcpus()

[Michael Roth]

Quoting Andrew Jones (2018-08-31 06:22:19)
> On Thu, Aug 30, 2018 at 05:51:13PM +0200, Laszlo Ersek wrote:
> > 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?
> 
> Yup. This guest agent functionality was merely a stopgap to support "cpu
> hotplug", before real cpu hotplug could be supported.
> 
> > 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).
> 
> That sounds good to me.

+1, otherwise we end up reporting unplugged VCPUs as being present-but-offline
by virtue of them being in the returned list, which isn't really the case.

> 
> Thanks,
> drew
> 

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

[Peter Krempa]

[-- Attachment #1: Type: text/plain, Size: 5473 bytes --]

On Fri, Aug 31, 2018 at 12:19:12 +0200, Laszlo Ersek wrote:
> On 08/31/18 10:00, Igor Mammedov wrote:
> > On Thu, 30 Aug 2018 17:51:13 +0200
> > Laszlo Ersek <lersek@redhat.com> 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@redhat.com>
> >>> ---
> >>>  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.

We do that. Not the unchanged part, but we request the structure, modify
some bits and pass it to the 'set' API.

> > Libvirt it seems that survives such outrageous output

Depending on the API. The old API which only allows to set a target vCPU
count has a check for it:

1630         /* This shouldn't happen, but we can't trust the guest agent */
1631         if (!cpuinfo[i].online && !cpuinfo[i].offlinable) {
1632             virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
1633                            _("Invalid data provided by guest agent"));
1634             return -1;
1635         }

The new API does not need such a check for all cases, but only for those
where the user requests a change. We don't check that the vCPU cannot be
onlined though in that case.

> >> 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).

I think that is the correct idea.

> > 
> >> 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 :)

I don't see a reason to disallow it since it is technically possible.

While it makes slightly less sense to use rather than real unplug since
the changes can be undone by the system administrator, but nevertheless
there are few advantages for trusted VMs e.g. boot cpu(s) are not
unpluggable by the new API and this approach allows to control them.

Additionally we don't know which approach the user wishes to use in most
cases so there's no magic point where we can disable the guest-agent
based API.

Note that the new API also reports the presence of the structure for the
given vCPU so there should not be any usability problem.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH] qga: ignore non present cpus when handling qmp_guest_get_vcpus()

[Igor Mammedov]

On Thu, 30 Aug 2018 17:51:13 +0200
Laszlo Ersek <lersek@redhat.com> 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@redhat.com>
> > ---
> >  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;
> >   
[...]
 
> 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).
would something like this on top of this patch do?

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 2929872..990bb80 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -2114,12 +2114,14 @@ GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
         vcpu->logical_id = current++;
         vcpu->has_can_offline = true; /* lolspeak ftw */
         transfer_vcpu(vcpu, true, &local_err);
-
-        entry = g_malloc0(sizeof *entry);
-        entry->value = vcpu;
-
-        *link = entry;
-        link = &entry->next;
+        if (errno == ENOENT) {
+            g_free(vcpu);
+        } else {
+            entry = g_malloc0(sizeof *entry);
+            entry->value = vcpu;
+            *link = entry;
+            link = &entry->next;
+        }
     }
 
     if (local_err == NULL) {

[...]

Re: [Qemu-devel] [PATCH] qga: ignore non present cpus when handling qmp_guest_get_vcpus()

[Laszlo Ersek]

On 09/06/18 11:49, Igor Mammedov wrote:
> On Thu, 30 Aug 2018 17:51:13 +0200
> Laszlo Ersek <lersek@redhat.com> 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@redhat.com>
>>> ---
>>>  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;
>>>   
> [...]
>  
>> 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).
> would something like this on top of this patch do?
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 2929872..990bb80 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -2114,12 +2114,14 @@ GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
>          vcpu->logical_id = current++;
>          vcpu->has_can_offline = true; /* lolspeak ftw */
>          transfer_vcpu(vcpu, true, &local_err);
> -
> -        entry = g_malloc0(sizeof *entry);
> -        entry->value = vcpu;
> -
> -        *link = entry;
> -        link = &entry->next;
> +        if (errno == ENOENT) {
> +            g_free(vcpu);
> +        } else {
> +            entry = g_malloc0(sizeof *entry);
> +            entry->value = vcpu;
> +            *link = entry;
> +            link = &entry->next;
> +        }
>      }
>  
>      if (local_err == NULL) {
> 
> [...]
> 

To me that looks like the right approach, but the details should be
polished a bit:

- After we drop the vcpu object, "local_err" is still set, and would
terminate the loop in the next iteration.

- It seems like ENOENT can indeed only come from openat(), in
transfer_vcpu(), however, it would be nice if we could grab the error
code from the error object somehow, and not from the "errno" variable. I
vaguely recall this is what error classes were originally invented for,
but now we just use ERROR_CLASS_GENERIC_ERROR...

How about this: we could add a boolean output param to transfer_vcpu(),
called "fatal". Ignored when the function succeeds. When the function
fails (seen from "local_err"), the loop consults "fatal". If the error
is fatal, we act as before; otherwise, we drop the vcpu object, release
-- and zero out -- "local_err" as well, and continue. I think this is
more generic / safer than trying to infer the failure location from the
outside.

I'm not quite up to date on structured error propagation in QEMU, so
take the above with a grain of salt...

Thanks,
Laszlo

Re: [Qemu-devel] [PATCH] qga: ignore non present cpus when handling qmp_guest_get_vcpus()

[Igor Mammedov]

On Thu, 6 Sep 2018 12:26:12 +0200
Laszlo Ersek <lersek@redhat.com> wrote:

> On 09/06/18 11:49, Igor Mammedov wrote:
> > On Thu, 30 Aug 2018 17:51:13 +0200
> > Laszlo Ersek <lersek@redhat.com> 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@redhat.com>
> >>> ---
> >>>  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;
> >>>     
> > [...]
> >    
> >> 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).  
> > would something like this on top of this patch do?
> > 
> > diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> > index 2929872..990bb80 100644
> > --- a/qga/commands-posix.c
> > +++ b/qga/commands-posix.c
> > @@ -2114,12 +2114,14 @@ GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
> >          vcpu->logical_id = current++;
> >          vcpu->has_can_offline = true; /* lolspeak ftw */
> >          transfer_vcpu(vcpu, true, &local_err);
> > -
> > -        entry = g_malloc0(sizeof *entry);
> > -        entry->value = vcpu;
> > -
> > -        *link = entry;
> > -        link = &entry->next;
> > +        if (errno == ENOENT) {
> > +            g_free(vcpu);
> > +        } else {
> > +            entry = g_malloc0(sizeof *entry);
> > +            entry->value = vcpu;
> > +            *link = entry;
> > +            link = &entry->next;
> > +        }
> >      }
> >  
> >      if (local_err == NULL) {
> > 
> > [...]
> >   
> 
> To me that looks like the right approach, but the details should be
> polished a bit:
> 
> - After we drop the vcpu object, "local_err" is still set, and would
> terminate the loop in the next iteration.
local_error is not set due 'if (!(sys2vcpu && errno == ENOENT))'
condition in the in the transfer_vcpu().
the thing is that in case of vcpu2sys direction ENOENT is hard error.

> - It seems like ENOENT can indeed only come from openat(), in
> transfer_vcpu(), however, it would be nice if we could grab the error
> code from the error object somehow, and not from the "errno" variable. I
> vaguely recall this is what error classes were originally invented for,
> but now we just use ERROR_CLASS_GENERIC_ERROR...
I've checked it and errno is preserved during error_setg_errno() call but
not saved in Error, so I've dropped that idea.
 
> How about this: we could add a boolean output param to transfer_vcpu(),
> called "fatal". Ignored when the function succeeds. When the function
> fails (seen from "local_err"), the loop consults "fatal". If the error
> is fatal, we act as before; otherwise, we drop the vcpu object, release
> -- and zero out -- "local_err" as well, and continue. I think this is
> more generic / safer than trying to infer the failure location from the
> outside.
It looked more uglier to me, so I've turned to libc style of reporting
(assuming that g_free() doesn't touch errno ever)

But if you prefer using extra parameter, I'll respin patch with it.

> I'm not quite up to date on structured error propagation in QEMU, so
> take the above with a grain of salt...
> 
> Thanks,
> Laszlo

Re: [Qemu-devel] [PATCH] qga: ignore non present cpus when handling qmp_guest_get_vcpus()

[Laszlo Ersek]

On 09/06/18 12:50, Igor Mammedov wrote:
> On Thu, 6 Sep 2018 12:26:12 +0200
> Laszlo Ersek <lersek@redhat.com> wrote:
> 
>> On 09/06/18 11:49, Igor Mammedov wrote:
>>> On Thu, 30 Aug 2018 17:51:13 +0200
>>> Laszlo Ersek <lersek@redhat.com> 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@redhat.com>
>>>>> ---
>>>>>  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;
>>>>>     
>>> [...]
>>>    
>>>> 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).  
>>> would something like this on top of this patch do?
>>>
>>> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>>> index 2929872..990bb80 100644
>>> --- a/qga/commands-posix.c
>>> +++ b/qga/commands-posix.c
>>> @@ -2114,12 +2114,14 @@ GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
>>>          vcpu->logical_id = current++;
>>>          vcpu->has_can_offline = true; /* lolspeak ftw */
>>>          transfer_vcpu(vcpu, true, &local_err);
>>> -
>>> -        entry = g_malloc0(sizeof *entry);
>>> -        entry->value = vcpu;
>>> -
>>> -        *link = entry;
>>> -        link = &entry->next;
>>> +        if (errno == ENOENT) {
>>> +            g_free(vcpu);
>>> +        } else {
>>> +            entry = g_malloc0(sizeof *entry);
>>> +            entry->value = vcpu;
>>> +            *link = entry;
>>> +            link = &entry->next;
>>> +        }
>>>      }
>>>  
>>>      if (local_err == NULL) {
>>>
>>> [...]
>>>   
>>
>> To me that looks like the right approach, but the details should be
>> polished a bit:
>>
>> - After we drop the vcpu object, "local_err" is still set, and would
>> terminate the loop in the next iteration.
> local_error is not set due 'if (!(sys2vcpu && errno == ENOENT))'
> condition in the in the transfer_vcpu().

ah, sorry, you did say this was on top of your original patch, but I had
forgotten the details of that.

> the thing is that in case of vcpu2sys direction ENOENT is hard error.
> 
>> - It seems like ENOENT can indeed only come from openat(), in
>> transfer_vcpu(), however, it would be nice if we could grab the error
>> code from the error object somehow, and not from the "errno" variable. I
>> vaguely recall this is what error classes were originally invented for,
>> but now we just use ERROR_CLASS_GENERIC_ERROR...
> I've checked it and errno is preserved during error_setg_errno() call but
> not saved in Error, so I've dropped that idea.
>  
>> How about this: we could add a boolean output param to transfer_vcpu(),
>> called "fatal". Ignored when the function succeeds. When the function
>> fails (seen from "local_err"), the loop consults "fatal". If the error
>> is fatal, we act as before; otherwise, we drop the vcpu object, release
>> -- and zero out -- "local_err" as well, and continue. I think this is
>> more generic / safer than trying to infer the failure location from the
>> outside.
> It looked more uglier to me, so I've turned to libc style of reporting
> (assuming that g_free() doesn't touch errno ever)
> 
> But if you prefer using extra parameter, I'll respin patch with it.

Michael, what is your preference? I guess I'll be fine both ways.

Thanks,
Laszlo

> 
>> I'm not quite up to date on structured error propagation in QEMU, so
>> take the above with a grain of salt...
>>
>> Thanks,
>> Laszlo
> 

Re: [Qemu-devel] [PATCH] qga: ignore non present cpus when handling qmp_guest_get_vcpus()

[Igor Mammedov]

On Thu, 6 Sep 2018 13:52:49 +0200
Laszlo Ersek <lersek@redhat.com> wrote:

> On 09/06/18 12:50, Igor Mammedov wrote:
> > On Thu, 6 Sep 2018 12:26:12 +0200
> > Laszlo Ersek <lersek@redhat.com> wrote:
> >   
> >> On 09/06/18 11:49, Igor Mammedov wrote:  
> >>> On Thu, 30 Aug 2018 17:51:13 +0200
> >>> Laszlo Ersek <lersek@redhat.com> 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@redhat.com>
> >>>>> ---
> >>>>>  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;
> >>>>>       
> >>> [...]
> >>>      
> >>>> 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).    
> >>> would something like this on top of this patch do?
> >>>
> >>> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> >>> index 2929872..990bb80 100644
> >>> --- a/qga/commands-posix.c
> >>> +++ b/qga/commands-posix.c
> >>> @@ -2114,12 +2114,14 @@ GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
> >>>          vcpu->logical_id = current++;
> >>>          vcpu->has_can_offline = true; /* lolspeak ftw */
> >>>          transfer_vcpu(vcpu, true, &local_err);
> >>> -
> >>> -        entry = g_malloc0(sizeof *entry);
> >>> -        entry->value = vcpu;
> >>> -
> >>> -        *link = entry;
> >>> -        link = &entry->next;
> >>> +        if (errno == ENOENT) {
> >>> +            g_free(vcpu);
> >>> +        } else {
> >>> +            entry = g_malloc0(sizeof *entry);
> >>> +            entry->value = vcpu;
> >>> +            *link = entry;
> >>> +            link = &entry->next;
> >>> +        }
> >>>      }
> >>>  
> >>>      if (local_err == NULL) {
> >>>
> >>> [...]
> >>>     
> >>
> >> To me that looks like the right approach, but the details should be
> >> polished a bit:
> >>
> >> - After we drop the vcpu object, "local_err" is still set, and would
> >> terminate the loop in the next iteration.  
> > local_error is not set due 'if (!(sys2vcpu && errno == ENOENT))'
> > condition in the in the transfer_vcpu().  
> 
> ah, sorry, you did say this was on top of your original patch, but I had
> forgotten the details of that.
> 
> > the thing is that in case of vcpu2sys direction ENOENT is hard error.
> >   
> >> - It seems like ENOENT can indeed only come from openat(), in
> >> transfer_vcpu(), however, it would be nice if we could grab the error
> >> code from the error object somehow, and not from the "errno" variable. I
> >> vaguely recall this is what error classes were originally invented for,
> >> but now we just use ERROR_CLASS_GENERIC_ERROR...  
> > I've checked it and errno is preserved during error_setg_errno() call but
> > not saved in Error, so I've dropped that idea.
> >    
> >> How about this: we could add a boolean output param to transfer_vcpu(),
> >> called "fatal". Ignored when the function succeeds. When the function
> >> fails (seen from "local_err"), the loop consults "fatal". If the error
> >> is fatal, we act as before; otherwise, we drop the vcpu object, release
> >> -- and zero out -- "local_err" as well, and continue. I think this is
> >> more generic / safer than trying to infer the failure location from the
> >> outside.  
> > It looked more uglier to me, so I've turned to libc style of reporting
> > (assuming that g_free() doesn't touch errno ever)
> > 
> > But if you prefer using extra parameter, I'll respin patch with it.
instead of inventing not really error parameter
we could move dirpath outside of transfer_vcpu().
It's bigger patch due to code movement but more straightforward logic.
I'll send v2 after testing.
 
> Michael, what is your preference? I guess I'll be fine both ways.
> 
> Thanks,
> Laszlo
> 
> >   
> >> I'm not quite up to date on structured error propagation in QEMU, so
> >> take the above with a grain of salt...
> >>
> >> Thanks,
> >> Laszlo  
> >   
> 
>