1. Patchew
  2. QEMU
  3. View series

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

Lin Ma posted 1 patch 3 years, 5 months ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20201120122702.4413-1-lma@suse.com
Maintainers: Michael Roth <mdroth@linux.vnet.ibm.com>
There is a newer version of this series
qga/commands-posix.c | 45 ++++++++++++++++----------------------------
1 file changed, 16 insertions(+), 29 deletions(-)
[PATCH v3] qga: Correct loop count in qmp_guest_get_vcpus()
Posted by Lin Ma 3 years, 5 months ago 
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>
---
 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");
+    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 */
@@ -2491,8 +2478,8 @@ GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
             *link = entry;
             link = &entry->next;
         }
-        g_free(path);
     }
+    g_dir_close(cpu_gdir);
 
     if (local_err == NULL) {
         /* there's no guest with zero VCPUs */
-- 
2.26.0
Re: [PATCH v3] qga: Correct loop count in qmp_guest_get_vcpus()
Posted by Marc-André Lureau 3 years, 5 months ago 
Hi

On Fri, Nov 20, 2020 at 4:27 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
> {"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 | 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?

+    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 */
> @@ -2491,8 +2478,8 @@ GuestLogicalProcessorList *qmp_guest_get_vcpus(Error
> **errp)
>              *link = entry;
>              link = &entry->next;
>          }
> -        g_free(path);
>      }
> +    g_dir_close(cpu_gdir);
>
>      if (local_err == NULL) {
>          /* there's no guest with zero VCPUs */
> --
> 2.26.0
>
>
thanks for the changes, that seems better to me

-- 
Marc-André Lureau
Re: [PATCH v3] qga: Correct loop count in qmp_guest_get_vcpus()
Posted by Philippe Mathieu-Daudé 3 years, 4 months ago 
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.
Re: [PATCH v3] qga: Correct loop count in qmp_guest_get_vcpus()
Posted by Peter Maydell 3 years, 4 months ago 
On Tue, 24 Nov 2020 at 14:12, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> As we are going to tag 5.2-rc3, what is the status of this fix?

'git blame' says none of the code changed here is newer than 2018,
so it seems unlikely that this is a regression since 5.1. rc3 is
now ready to tag, so this is going to get postponed to 6.0
unless somebody has a really solid argument for why it is
a release-critical bug.

thanks
-- PMM

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.