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

[Philippe Mathieu-Daudé]

Hi,

On 11/20/20 1:33 PM, Marc-André Lureau wrote:
> Hi
> 
> On Fri, Nov 20, 2020 at 4:27 PM Lin Ma <lma@suse.com
> <mailto: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
>     {"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 <mailto:lma@suse.com>>
>     ---
>      qga/commands-posix.c | 45 ++++++++++++++++----------------------------
>      1 file changed, 16 insertions(+), 29 deletions(-)
> 
>     diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>     index c089e38120..ee05e694d3 100644
>     --- a/qga/commands-posix.c
>     +++ b/qga/commands-posix.c
>     @@ -2376,24 +2376,6 @@ error:
>          return NULL;
>      }
> 
>     -#define SYSCONF_EXACT(name, errp) sysconf_exact((name), #name, (errp))
>     -
>     -static long sysconf_exact(int name, const char *name_str, Error **errp)
>     -{
>     -    long ret;
>     -
>     -    errno = 0;
>     -    ret = sysconf(name);
>     -    if (ret == -1) {
>     -        if (errno == 0) {
>     -            error_setg(errp, "sysconf(%s): value indefinite",
>     name_str);
>     -        } else {
>     -            error_setg_errno(errp, errno, "sysconf(%s)", name_str);
>     -        }
>     -    }
>     -    return ret;
>     -}
>     -
>      /* Transfer online/offline status between @vcpu and the guest system.
>       *
>       * On input either @errp or *@errp must be NULL.
>     @@ -2464,24 +2446,29 @@ static void
>     transfer_vcpu(GuestLogicalProcessor *vcpu, bool sys2vcpu,
> 
>      GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
>      {
>     -    int64_t current;
>          GuestLogicalProcessorList *head, **link;
>     -    long sc_max;
>     +    g_autofree char *cpu_dir = NULL;
>     +    const gchar *line;
>     +    GDir *cpu_gdir = NULL;
>          Error *local_err = NULL;
> 
>     -    current = 0;
>          head = NULL;
>          link = &head;
>     -    sc_max = SYSCONF_EXACT(_SC_NPROCESSORS_CONF, &local_err);
>     +    cpu_dir = g_strdup_printf("/sys/devices/system/cpu");
> 
> 
> Why strdup?

As we are going to tag 5.2-rc3, what is the status of this fix?

Thanks,

Phil.