diff mbox series

qga: Correct loop count in qmp_guest_get_vcpus()

Message ID 20201119084557.27870-1-lma@suse.com (mailing list archive)
State New, archived
Headers show
Series qga: Correct loop count in qmp_guest_get_vcpus() | expand

Commit Message

Lin Ma Nov. 19, 2020, 8:45 a.m. UTC
The guest-get-vcpus returns incorrect vcpu info in case we hotunplug vcpus(not
the last one).
e.g.:
A VM has 4 VCPUs: cpu0 + 3 hotunpluggable online vcpus(cpu1, cpu2 and cpu3).
Hotunplug cpu2,  Now only cpu0, cpu1 and cpu3 are present & online.

./qmp-shell /tmp/qmp-monitor.sock
(QEMU) query-hotpluggable-cpus
{"return": [
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 3}, "vcpus-count": 1,
 "qom-path": "/machine/peripheral/cpu3", "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 2}, "vcpus-count": 1,
 "qom-path": "/machine/peripheral/cpu2", "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 1}, "vcpus-count": 1,
 "qom-path": "/machine/peripheral/cpu1", "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 0}, "vcpus-count": 1,
 "qom-path": "/machine/unattached/device[0]", "type": "host-x86_64-cpu"}
]}

(QEMU) device_del id=cpu2
{"return": {}}

(QEMU) query-hotpluggable-cpus
{"return": [
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 3}, "vcpus-count": 1,
 "qom-path": "/machine/peripheral/cpu3", "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 2}, "vcpus-count": 1,
 "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 1}, "vcpus-count": 1,
 "qom-path": "/machine/peripheral/cpu1", "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 0}, "vcpus-count": 1,
 "qom-path": "/machine/unattached/device[0]", "type": "host-x86_64-cpu"}
]}

Before:
./qmp-shell -N /tmp/qmp-ga.sock
Welcome to the QMP low-level shell!
Connected
(QEMU) guest-get-vcpus
{"return": [
{"online": true, "can-offline": false, "logical-id": 0},
{"online": true, "can-offline": true, "logical-id": 1}]}

After:
./qmp-shell -N /tmp/qmp-ga.sock
Welcome to the QMP low-level shell!
Connected
(QEMU) guest-get-vcpus
{"execute":"guest-get-vcpus"}
{"return": [
{"online": true, "can-offline": false, "logical-id": 0},
{"online": true, "can-offline": true, "logical-id": 1},
{"online": true, "can-offline": true, "logical-id": 3}]}

Signed-off-by: Lin Ma <lma@suse.com>
---
 qga/commands-posix.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Comments

Marc-André Lureau Nov. 19, 2020, 2:46 p.m. UTC | #1
Hi

On Thu, Nov 19, 2020 at 12:48 PM Lin Ma <lma@suse.com> wrote:

> The guest-get-vcpus returns incorrect vcpu info in case we hotunplug
> vcpus(not
> the last one).
> e.g.:
> A VM has 4 VCPUs: cpu0 + 3 hotunpluggable online vcpus(cpu1, cpu2 and
> cpu3).
> Hotunplug cpu2,  Now only cpu0, cpu1 and cpu3 are present & online.
>
> ./qmp-shell /tmp/qmp-monitor.sock
> (QEMU) query-hotpluggable-cpus
> {"return": [
> {"props": {"core-id": 0, "thread-id": 0, "socket-id": 3}, "vcpus-count": 1,
>  "qom-path": "/machine/peripheral/cpu3", "type": "host-x86_64-cpu"},
> {"props": {"core-id": 0, "thread-id": 0, "socket-id": 2}, "vcpus-count": 1,
>  "qom-path": "/machine/peripheral/cpu2", "type": "host-x86_64-cpu"},
> {"props": {"core-id": 0, "thread-id": 0, "socket-id": 1}, "vcpus-count": 1,
>  "qom-path": "/machine/peripheral/cpu1", "type": "host-x86_64-cpu"},
> {"props": {"core-id": 0, "thread-id": 0, "socket-id": 0}, "vcpus-count": 1,
>  "qom-path": "/machine/unattached/device[0]", "type": "host-x86_64-cpu"}
> ]}
>
> (QEMU) device_del id=cpu2
> {"return": {}}
>
> (QEMU) query-hotpluggable-cpus
> {"return": [
> {"props": {"core-id": 0, "thread-id": 0, "socket-id": 3}, "vcpus-count": 1,
>  "qom-path": "/machine/peripheral/cpu3", "type": "host-x86_64-cpu"},
> {"props": {"core-id": 0, "thread-id": 0, "socket-id": 2}, "vcpus-count": 1,
>  "type": "host-x86_64-cpu"},
> {"props": {"core-id": 0, "thread-id": 0, "socket-id": 1}, "vcpus-count": 1,
>  "qom-path": "/machine/peripheral/cpu1", "type": "host-x86_64-cpu"},
> {"props": {"core-id": 0, "thread-id": 0, "socket-id": 0}, "vcpus-count": 1,
>  "qom-path": "/machine/unattached/device[0]", "type": "host-x86_64-cpu"}
> ]}
>
> Before:
> ./qmp-shell -N /tmp/qmp-ga.sock
> Welcome to the QMP low-level shell!
> Connected
> (QEMU) guest-get-vcpus
> {"return": [
> {"online": true, "can-offline": false, "logical-id": 0},
> {"online": true, "can-offline": true, "logical-id": 1}]}
>
> After:
> ./qmp-shell -N /tmp/qmp-ga.sock
> Welcome to the QMP low-level shell!
> Connected
> (QEMU) guest-get-vcpus
> {"execute":"guest-get-vcpus"}
> {"return": [
> {"online": true, "can-offline": false, "logical-id": 0},
> {"online": true, "can-offline": true, "logical-id": 1},
> {"online": true, "can-offline": true, "logical-id": 3}]}
>
> Signed-off-by: Lin Ma <lma@suse.com>
> ---
>  qga/commands-posix.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 3bffee99d4..accc893373 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -2182,15 +2182,15 @@ GuestLogicalProcessorList
> *qmp_guest_get_vcpus(Error **errp)
>  {
>      int64_t current;
>      GuestLogicalProcessorList *head, **link;
> -    long sc_max;
> +    long max_loop_count;
>      Error *local_err = NULL;
>
>      current = 0;
>      head = NULL;
>      link = &head;
> -    sc_max = SYSCONF_EXACT(_SC_NPROCESSORS_CONF, &local_err);
> +    max_loop_count = SYSCONF_EXACT(_SC_NPROCESSORS_CONF, &local_err);
>
> -    while (local_err == NULL && current < sc_max) {
> +    while (local_err == NULL && current < max_loop_count) {
>          GuestLogicalProcessor *vcpu;
>          GuestLogicalProcessorList *entry;
>          int64_t id = current++;
> @@ -2206,6 +2206,8 @@ GuestLogicalProcessorList *qmp_guest_get_vcpus(Error
> **errp)
>              entry->value = vcpu;
>              *link = entry;
>              link = &entry->next;
> +        } else {
> +            max_loop_count += 1;
>

This looks like a recipe for infinite loop on error.

Shouldn't we loop over all the /sys/devices/system/cpu/cpu#/ instead?

(possibly parse /sys/devices/system/cpu/present, but I doubt it's necessary)
lma Nov. 20, 2020, 9:28 a.m. UTC | #2
On 2020-11-19 14:46, Marc-André Lureau wrote:
> Hi
> 
> On Thu, Nov 19, 2020 at 12:48 PM Lin Ma <lma@suse.com> wrote:
> 
>> The guest-get-vcpus returns incorrect vcpu info in case we hotunplug
>> vcpus(not
>> the last one).
>> e.g.:
>> A VM has 4 VCPUs: cpu0 + 3 hotunpluggable online vcpus(cpu1, cpu2 and
>> cpu3).
>> Hotunplug cpu2,  Now only cpu0, cpu1 and cpu3 are present & online.
>> 
>> ./qmp-shell /tmp/qmp-monitor.sock
>> (QEMU) query-hotpluggable-cpus
>> {"return": [
>> {"props": {"core-id": 0, "thread-id": 0, "socket-id": 3}, 
>> "vcpus-count": 1,
>>  "qom-path": "/machine/peripheral/cpu3", "type": "host-x86_64-cpu"},
>> {"props": {"core-id": 0, "thread-id": 0, "socket-id": 2}, 
>> "vcpus-count": 1,
>>  "qom-path": "/machine/peripheral/cpu2", "type": "host-x86_64-cpu"},
>> {"props": {"core-id": 0, "thread-id": 0, "socket-id": 1}, 
>> "vcpus-count": 1,
>>  "qom-path": "/machine/peripheral/cpu1", "type": "host-x86_64-cpu"},
>> {"props": {"core-id": 0, "thread-id": 0, "socket-id": 0}, 
>> "vcpus-count": 1,
>>  "qom-path": "/machine/unattached/device[0]", "type": 
>> "host-x86_64-cpu"}
>> ]}
>> 
>> (QEMU) device_del id=cpu2
>> {"return": {}}
>> 
>> (QEMU) query-hotpluggable-cpus
>> {"return": [
>> {"props": {"core-id": 0, "thread-id": 0, "socket-id": 3}, 
>> "vcpus-count": 1,
>>  "qom-path": "/machine/peripheral/cpu3", "type": "host-x86_64-cpu"},
>> {"props": {"core-id": 0, "thread-id": 0, "socket-id": 2}, 
>> "vcpus-count": 1,
>>  "type": "host-x86_64-cpu"},
>> {"props": {"core-id": 0, "thread-id": 0, "socket-id": 1}, 
>> "vcpus-count": 1,
>>  "qom-path": "/machine/peripheral/cpu1", "type": "host-x86_64-cpu"},
>> {"props": {"core-id": 0, "thread-id": 0, "socket-id": 0}, 
>> "vcpus-count": 1,
>>  "qom-path": "/machine/unattached/device[0]", "type": 
>> "host-x86_64-cpu"}
>> ]}
>> 
>> Before:
>> ./qmp-shell -N /tmp/qmp-ga.sock
>> Welcome to the QMP low-level shell!
>> Connected
>> (QEMU) guest-get-vcpus
>> {"return": [
>> {"online": true, "can-offline": false, "logical-id": 0},
>> {"online": true, "can-offline": true, "logical-id": 1}]}
>> 
>> After:
>> ./qmp-shell -N /tmp/qmp-ga.sock
>> Welcome to the QMP low-level shell!
>> Connected
>> (QEMU) guest-get-vcpus
>> {"execute":"guest-get-vcpus"}
>> {"return": [
>> {"online": true, "can-offline": false, "logical-id": 0},
>> {"online": true, "can-offline": true, "logical-id": 1},
>> {"online": true, "can-offline": true, "logical-id": 3}]}
>> 
>> Signed-off-by: Lin Ma <lma@suse.com>
>> ---
>>  qga/commands-posix.c | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>> 
>> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>> index 3bffee99d4..accc893373 100644
>> --- a/qga/commands-posix.c
>> +++ b/qga/commands-posix.c
>> @@ -2182,15 +2182,15 @@ GuestLogicalProcessorList
>> *qmp_guest_get_vcpus(Error **errp)
>>  {
>>      int64_t current;
>>      GuestLogicalProcessorList *head, **link;
>> -    long sc_max;
>> +    long max_loop_count;
>>      Error *local_err = NULL;
>> 
>>      current = 0;
>>      head = NULL;
>>      link = &head;
>> -    sc_max = SYSCONF_EXACT(_SC_NPROCESSORS_CONF, &local_err);
>> +    max_loop_count = SYSCONF_EXACT(_SC_NPROCESSORS_CONF, &local_err);
>> 
>> -    while (local_err == NULL && current < sc_max) {
>> +    while (local_err == NULL && current < max_loop_count) {
>>          GuestLogicalProcessor *vcpu;
>>          GuestLogicalProcessorList *entry;
>>          int64_t id = current++;
>> @@ -2206,6 +2206,8 @@ GuestLogicalProcessorList 
>> *qmp_guest_get_vcpus(Error
>> **errp)
>>              entry->value = vcpu;
>>              *link = entry;
>>              link = &entry->next;
>> +        } else {
>> +            max_loop_count += 1;
>> 
> 
> This looks like a recipe for infinite loop on error.
Emm...It is possible.
> 
> Shouldn't we loop over all the /sys/devices/system/cpu/cpu#/ instead?
Originally I'd like to use the function fnmatch to handle pattern cpu# 
to
loop over all of the /sys/devices/system/cpu/cpu#/, But it introduces 
the
header file fnmatch.h and make things complicated a little.

> 
> (possibly parse /sys/devices/system/cpu/present, but I doubt it's 
> necessary)
IMO the 'present' won't help.

I'm about to post the V2, I made tiny change in the V2, Please help to 
review.

BTW, The local_err will be set in case of error, right? It could avoid 
infinite loop.

Thanks a lot,
Lin
Marc-André Lureau Nov. 20, 2020, 9:48 a.m. UTC | #3
Hi

On Fri, Nov 20, 2020 at 1:28 PM Lin Ma <lma@suse.de> wrote:

> On 2020-11-19 14:46, Marc-André Lureau wrote:
> > Hi
> >
> > On Thu, Nov 19, 2020 at 12:48 PM Lin Ma <lma@suse.com> wrote:
> >
> >> The guest-get-vcpus returns incorrect vcpu info in case we hotunplug
> >> vcpus(not
> >> the last one).
> >> e.g.:
> >> A VM has 4 VCPUs: cpu0 + 3 hotunpluggable online vcpus(cpu1, cpu2 and
> >> cpu3).
> >> Hotunplug cpu2,  Now only cpu0, cpu1 and cpu3 are present & online.
> >>
> >> ./qmp-shell /tmp/qmp-monitor.sock
> >> (QEMU) query-hotpluggable-cpus
> >> {"return": [
> >> {"props": {"core-id": 0, "thread-id": 0, "socket-id": 3},
> >> "vcpus-count": 1,
> >>  "qom-path": "/machine/peripheral/cpu3", "type": "host-x86_64-cpu"},
> >> {"props": {"core-id": 0, "thread-id": 0, "socket-id": 2},
> >> "vcpus-count": 1,
> >>  "qom-path": "/machine/peripheral/cpu2", "type": "host-x86_64-cpu"},
> >> {"props": {"core-id": 0, "thread-id": 0, "socket-id": 1},
> >> "vcpus-count": 1,
> >>  "qom-path": "/machine/peripheral/cpu1", "type": "host-x86_64-cpu"},
> >> {"props": {"core-id": 0, "thread-id": 0, "socket-id": 0},
> >> "vcpus-count": 1,
> >>  "qom-path": "/machine/unattached/device[0]", "type":
> >> "host-x86_64-cpu"}
> >> ]}
> >>
> >> (QEMU) device_del id=cpu2
> >> {"return": {}}
> >>
> >> (QEMU) query-hotpluggable-cpus
> >> {"return": [
> >> {"props": {"core-id": 0, "thread-id": 0, "socket-id": 3},
> >> "vcpus-count": 1,
> >>  "qom-path": "/machine/peripheral/cpu3", "type": "host-x86_64-cpu"},
> >> {"props": {"core-id": 0, "thread-id": 0, "socket-id": 2},
> >> "vcpus-count": 1,
> >>  "type": "host-x86_64-cpu"},
> >> {"props": {"core-id": 0, "thread-id": 0, "socket-id": 1},
> >> "vcpus-count": 1,
> >>  "qom-path": "/machine/peripheral/cpu1", "type": "host-x86_64-cpu"},
> >> {"props": {"core-id": 0, "thread-id": 0, "socket-id": 0},
> >> "vcpus-count": 1,
> >>  "qom-path": "/machine/unattached/device[0]", "type":
> >> "host-x86_64-cpu"}
> >> ]}
> >>
> >> Before:
> >> ./qmp-shell -N /tmp/qmp-ga.sock
> >> Welcome to the QMP low-level shell!
> >> Connected
> >> (QEMU) guest-get-vcpus
> >> {"return": [
> >> {"online": true, "can-offline": false, "logical-id": 0},
> >> {"online": true, "can-offline": true, "logical-id": 1}]}
> >>
> >> After:
> >> ./qmp-shell -N /tmp/qmp-ga.sock
> >> Welcome to the QMP low-level shell!
> >> Connected
> >> (QEMU) guest-get-vcpus
> >> {"execute":"guest-get-vcpus"}
> >> {"return": [
> >> {"online": true, "can-offline": false, "logical-id": 0},
> >> {"online": true, "can-offline": true, "logical-id": 1},
> >> {"online": true, "can-offline": true, "logical-id": 3}]}
> >>
> >> Signed-off-by: Lin Ma <lma@suse.com>
> >> ---
> >>  qga/commands-posix.c | 8 +++++---
> >>  1 file changed, 5 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> >> index 3bffee99d4..accc893373 100644
> >> --- a/qga/commands-posix.c
> >> +++ b/qga/commands-posix.c
> >> @@ -2182,15 +2182,15 @@ GuestLogicalProcessorList
> >> *qmp_guest_get_vcpus(Error **errp)
> >>  {
> >>      int64_t current;
> >>      GuestLogicalProcessorList *head, **link;
> >> -    long sc_max;
> >> +    long max_loop_count;
> >>      Error *local_err = NULL;
> >>
> >>      current = 0;
> >>      head = NULL;
> >>      link = &head;
> >> -    sc_max = SYSCONF_EXACT(_SC_NPROCESSORS_CONF, &local_err);
> >> +    max_loop_count = SYSCONF_EXACT(_SC_NPROCESSORS_CONF, &local_err);
> >>
> >> -    while (local_err == NULL && current < sc_max) {
> >> +    while (local_err == NULL && current < max_loop_count) {
> >>          GuestLogicalProcessor *vcpu;
> >>          GuestLogicalProcessorList *entry;
> >>          int64_t id = current++;
> >> @@ -2206,6 +2206,8 @@ GuestLogicalProcessorList
> >> *qmp_guest_get_vcpus(Error
> >> **errp)
> >>              entry->value = vcpu;
> >>              *link = entry;
> >>              link = &entry->next;
> >> +        } else {
> >> +            max_loop_count += 1;
> >>
> >
> > This looks like a recipe for infinite loop on error.
> Emm...It is possible.
> >
> > Shouldn't we loop over all the /sys/devices/system/cpu/cpu#/ instead?
> Originally I'd like to use the function fnmatch to handle pattern cpu#
> to
> loop over all of the /sys/devices/system/cpu/cpu#/, But it introduces
> the
> header file fnmatch.h and make things complicated a little.
>
>
Why use fnmatch?
g_dir_open & g_dir_read_name, then you can sscanf for the matching entries.


> >
> > (possibly parse /sys/devices/system/cpu/present, but I doubt it's
> > necessary)
> IMO the 'present' won't help.
>
> I'm about to post the V2, I made tiny change in the V2, Please help to
> review.
>
> BTW, The local_err will be set in case of error, right? It could avoid
> infinite loop.
>
> I think it should.
diff mbox series

Patch

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 3bffee99d4..accc893373 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -2182,15 +2182,15 @@  GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
 {
     int64_t current;
     GuestLogicalProcessorList *head, **link;
-    long sc_max;
+    long max_loop_count;
     Error *local_err = NULL;
 
     current = 0;
     head = NULL;
     link = &head;
-    sc_max = SYSCONF_EXACT(_SC_NPROCESSORS_CONF, &local_err);
+    max_loop_count = SYSCONF_EXACT(_SC_NPROCESSORS_CONF, &local_err);
 
-    while (local_err == NULL && current < sc_max) {
+    while (local_err == NULL && current < max_loop_count) {
         GuestLogicalProcessor *vcpu;
         GuestLogicalProcessorList *entry;
         int64_t id = current++;
@@ -2206,6 +2206,8 @@  GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
             entry->value = vcpu;
             *link = entry;
             link = &entry->next;
+        } else {
+            max_loop_count += 1;
         }
         g_free(path);
     }