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

[Marc-André Lureau]

Hi

On Fri, Jan 29, 2021 at 1:00 PM lma--- via <qemu-devel@nongnu.org> 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>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  qga/commands-posix.c | 43 ++++++++++++++-----------------------------
>  1 file changed, 14 insertions(+), 29 deletions(-)
>

The patch no longer applies cleanly. Can you rebase?

thanks

> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index edf785b2da..646d9cebb5 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -2385,24 +2385,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.
> @@ -2473,24 +2455,28 @@ static void transfer_vcpu(GuestLogicalProcessor 
> *vcpu, bool sys2vcpu,
>
>  GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
>  {
> -    int64_t current;
>      GuestLogicalProcessorList *head, **link;
> -    long sc_max;
> +    const char *cpu_dir = "/sys/devices/system/cpu";
> +    const gchar *line;
> +    g_autoptr(GDir) cpu_gdir = NULL;
>      Error *local_err = NULL;
>
> -    current = 0;
>      head = NULL;
>      link = &head;
> -    sc_max = SYSCONF_EXACT(_SC_NPROCESSORS_CONF, &local_err);
> +    cpu_gdir = g_dir_open(cpu_dir, 0, NULL);
> +
> +    if (cpu_gdir == NULL) {
> +        error_setg_errno(errp, errno, "failed to list entries: %s", cpu_dir);
> +        return NULL;
> +    }
>
> -    while (local_err == NULL && current < sc_max) {
> +    while (local_err == NULL && (line = g_dir_read_name(cpu_gdir)) != NULL) {
>          GuestLogicalProcessor *vcpu;
>          GuestLogicalProcessorList *entry;
> -        int64_t id = current++;
> -        char *path = g_strdup_printf("/sys/devices/system/cpu/cpu%" PRId64 
> "/",
> -                                     id);
> -
> -        if (g_file_test(path, G_FILE_TEST_EXISTS)) {
> +        int64_t id;
> +        if (sscanf(line, "cpu%ld", &id)) {
> +            g_autofree char *path = 
> g_strdup_printf("/sys/devices/system/cpu/"
> +                                                    "cpu%" PRId64 "/", id);
>              vcpu = g_malloc0(sizeof *vcpu);
>              vcpu->logical_id = id;
>              vcpu->has_can_offline = true; /* lolspeak ftw */
> @@ -2500,7 +2486,6 @@ GuestLogicalProcessorList *qmp_guest_get_vcpus(Error 
> **errp)
>              *link = entry;
>              link = &entry->next;
>          }
> -        g_free(path);
>      }
>
>      if (local_err == NULL) {
> --
> 2.26.2
>
>


-- 
Marc-André Lureau