Re: [PATCH] qga: Correct loop count in qmp_guest_get_vcpus()

[Lin Ma]

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