| Message ID | 20201119084557.27870-1-lma@suse.com |
|---|---|
| State | New |
| Headers | show |
| Series | qga: Correct loop count in qmp_guest_get_vcpus() | expand |
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)
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
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 --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); }
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(-)