[Qemu-devel] [PATCH v2 0/3] qga/Linux: online/offline/query VCPUs via guest sysfs

[Qemu-devel] [PATCH v2 0/3] qga/Linux: online/offline/query VCPUs via guest sysfs

[Laszlo Ersek]

Until the uncomparably harder task of real VCPU hotplug / hot-unplug is
completed, here's a small guest agent series that imitates the same
thing through the sysfs of the Linux guest. We've heard that people
migrating from another VMM might be transitorily interested in this.

In v2: addressing the reviews of Eric and Michael (many thanks).
- Deal with non-offline-able VCPUs, introducing
  GuestLogicalProcessor.can_offline,
- POSIX write()-style retvals in guest-set-vcpus,
- move implementation into preexistent __linux__ section, keep
  standalone stubs,
- massage sysfs files with <unistd.h> interfaces.

(v2 is practically a rewrite; the interdiff doesn't appear particularly
helpful.)

Laszlo Ersek (3):
  qga: introduce guest-get-vcpus / guest-set-vcpus with stubs
  qga: implement qmp_guest_get_vcpus() for Linux with sysfs
  qga: implement qmp_guest_set_vcpus() for Linux with sysfs

 qga/qapi-schema.json |   72 +++++++++++++++++++++
 qga/commands-posix.c |  172 ++++++++++++++++++++++++++++++++++++++++++++++++++
 qga/commands-win32.c |   12 ++++
 3 files changed, 256 insertions(+), 0 deletions(-)

[Qemu-devel] [PATCH v2 1/3] qga: introduce guest-get-vcpus / guest-set-vcpus with stubs

[Laszlo Ersek]

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 qga/qapi-schema.json |   72 ++++++++++++++++++++++++++++++++++++++++++++++++++
 qga/commands-posix.c |   12 ++++++++
 qga/commands-win32.c |   12 ++++++++
 3 files changed, 96 insertions(+), 0 deletions(-)

diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index d91d903..cba881c 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -515,3 +515,75 @@
 ##
 { 'command': 'guest-network-get-interfaces',
   'returns': ['GuestNetworkInterface'] }
+
+##
+# @GuestLogicalProcessor:
+#
+# @logical-id: Arbitrary guest-specific unique identifier of the VCPU.
+#
+# @online: Whether the VCPU is enabled.
+#
+# @can-offline: Whether offlining the VCPU is possible. This member is always
+#               filled in by the guest agent when the structure is returned,
+#               and always ignored on input (hence it can be omitted then).
+#
+# Since: 1.5
+##
+{ 'type': 'GuestLogicalProcessor',
+  'data': {'logical-id': 'int',
+           'online': 'bool',
+           '*can-offline': 'bool'} }
+
+##
+# @guest-get-vcpus:
+#
+# Retrieve the list of the guest's logical processors.
+#
+# This is a read-only operation.
+#
+# Returns: The list of all VCPUs the guest knows about. Each VCPU is put on the
+# list exactly once, but their order is unspecified.
+#
+# Since: 1.5
+##
+{ 'command': 'guest-get-vcpus',
+  'returns': ['GuestLogicalProcessor'] }
+
+##
+# @guest-set-vcpus:
+#
+# Attempt to reconfigure (currently: enable/disable) logical processors inside
+# the guest.
+#
+# The input list is processed node by node in order. In each node @logical-id
+# is used to look up the guest VCPU, for which @online specifies the requested
+# state. The set of distinct @logical-id's is only required to be a subset of
+# the guest-supported identifiers. There's no restriction on list length or on
+# repeating the same @logical-id (with possibly different @online field).
+# Preferably the input list should describe a modified subset of
+# @guest-get-vcpus' return value.
+#
+# Returns: The length of the initial sublist that has been successfully
+#          processed. The guest agent maximizes this value. Possible cases:
+#
+#          0:                if the @vcpus list was empty on input. Guest state
+#                            has not been changed. Otherwise,
+#
+#          Error:            processing the first node of @vcpus failed for the
+#                            reason returned. Guest state has not been changed.
+#                            Otherwise,
+#
+#          < length(@vcpus): more than zero initial nodes have been processed,
+#                            but not the entire @vcpus list. Guest state has
+#                            changed accordingly. To retrieve the error
+#                            (assuming it persists), repeat the call with the
+#                            successfully processed initial sublist removed.
+#                            Otherwise,
+#
+#          length(@vcpus):   call successful.
+#
+# Since: 1.5
+##
+{ 'command': 'guest-set-vcpus',
+  'data':    {'vcpus': ['GuestLogicalProcessor'] },
+  'returns': 'int' }
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 7a0202e..7257145 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1083,6 +1083,18 @@ void qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **err)
 }
 #endif
 
+GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
+{
+    error_set(errp, QERR_UNSUPPORTED);
+    return NULL;
+}
+
+int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
+{
+    error_set(errp, QERR_UNSUPPORTED);
+    return -1;
+}
+
 /* register init/cleanup routines for stateful command groups */
 void ga_command_state_init(GAState *s, GACommandState *cs)
 {
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 7e8ecb3..7781205 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -278,6 +278,18 @@ GuestNetworkInterfaceList *qmp_guest_network_get_interfaces(Error **err)
     return NULL;
 }
 
+GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
+{
+    error_set(errp, QERR_UNSUPPORTED);
+    return NULL;
+}
+
+int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
+{
+    error_set(errp, QERR_UNSUPPORTED);
+    return -1;
+}
+
 /* register init/cleanup routines for stateful command groups */
 void ga_command_state_init(GAState *s, GACommandState *cs)
 {
-- 
1.7.1

[Qemu-devel] [PATCH v2 2/3] qga: implement qmp_guest_get_vcpus() for Linux with sysfs

[Laszlo Ersek]

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 qga/commands-posix.c |  146 +++++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 140 insertions(+), 6 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 7257145..fd38e14 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -15,6 +15,10 @@
 #include <sys/types.h>
 #include <sys/ioctl.h>
 #include <sys/wait.h>
+#include <unistd.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <inttypes.h>
 #include "qga/guest-agent-core.h"
 #include "qga-qmp-commands.h"
 #include "qapi/qmp/qerror.h"
@@ -1027,6 +1031,136 @@ error:
     return NULL;
 }
 
+#define SYSCONF_EXACT(name, err) sysconf_exact((name), #name, (err))
+
+static long sysconf_exact(int name, const char *name_str, Error **err)
+{
+    long ret;
+
+    errno = 0;
+    ret = sysconf(name);
+    if (ret == -1) {
+        if (errno == 0) {
+            error_setg(err, "sysconf(%s): value indefinite", name_str);
+        } else {
+            error_setg_errno(err, 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.
+ *
+ * In system-to-@vcpu direction, the following @vcpu fields are accessed:
+ * - R: vcpu->logical_id
+ * - W: vcpu->online
+ * - W: vcpu->can_offline
+ *
+ * In @vcpu-to-system direction, the following @vcpu fields are accessed:
+ * - R: vcpu->logical_id
+ * - R: vcpu->online
+ *
+ * Written members remain unmodified on error.
+ */
+static void transfer_vcpu(GuestLogicalProcessor *vcpu, bool sys2vcpu,
+                          Error **errp)
+{
+    char *dirpath;
+    int dirfd;
+
+    dirpath = g_strdup_printf("/sys/devices/system/cpu/cpu%" PRId64 "/",
+                              vcpu->logical_id);
+    dirfd = open(dirpath, O_RDONLY | O_DIRECTORY);
+    if (dirfd == -1) {
+        error_setg_errno(errp, errno, "open(\"%s\")", dirpath);
+    } else {
+        static const char fn[] = "online";
+        int fd;
+        int res;
+
+        fd = openat(dirfd, fn, sys2vcpu ? O_RDONLY : O_RDWR);
+        if (fd == -1) {
+            if (errno != ENOENT) {
+                error_setg_errno(errp, errno, "open(\"%s/%s\")", dirpath, fn);
+            } else if (sys2vcpu) {
+                vcpu->online = true;
+                vcpu->can_offline = false;
+            } else if (!vcpu->online) {
+                error_setg(errp, "logical processor #%" PRId64 " can't be "
+                           "offlined", vcpu->logical_id);
+            } /* otherwise pretend successful re-onlining */
+        } else {
+            unsigned char status;
+
+            res = pread(fd, &status, 1, 0);
+            if (res == -1) {
+                error_setg_errno(errp, errno, "pread(\"%s/%s\")", dirpath, fn);
+            } else if (res == 0) {
+                error_setg(errp, "pread(\"%s/%s\"): unexpected EOF", dirpath,
+                           fn);
+            } else if (sys2vcpu) {
+                vcpu->online = (status != '0');
+                vcpu->can_offline = true;
+            } else if (vcpu->online != (status != '0')) {
+                status = '0' + vcpu->online;
+                if (pwrite(fd, &status, 1, 0) == -1) {
+                    error_setg_errno(errp, errno, "pwrite(\"%s/%s\")", dirpath,
+                                     fn);
+                }
+            } /* otherwise pretend successful re-(on|off)-lining */
+
+            res = close(fd);
+            g_assert(res == 0);
+        }
+
+        res = close(dirfd);
+        g_assert(res == 0);
+    }
+
+    g_free(dirpath);
+}
+
+GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
+{
+    int64_t current;
+    GuestLogicalProcessorList *head, **link;
+    long sc_max;
+    Error *local_err = NULL;
+
+    current = 0;
+    head = NULL;
+    link = &head;
+    sc_max = SYSCONF_EXACT(_SC_NPROCESSORS_CONF, &local_err);
+
+    while (local_err == NULL && current < sc_max) {
+        GuestLogicalProcessor *vcpu;
+        GuestLogicalProcessorList *entry;
+
+        vcpu = g_malloc0(sizeof *vcpu);
+        vcpu->logical_id = current++;
+        vcpu->has_can_offline = true; /* lolspeak ftw */
+        transfer_vcpu(vcpu, true, &local_err);
+
+        entry = g_malloc0(sizeof *entry);
+        entry->value = vcpu;
+
+        *link = entry;
+        link = &entry->next;
+    }
+
+    if (local_err == NULL) {
+        /* there's no guest with zero VCPUs */
+        g_assert(head != NULL);
+        return head;
+    }
+
+    qapi_free_GuestLogicalProcessorList(head);
+    error_propagate(errp, local_err);
+    return NULL;
+}
+
 #else /* defined(__linux__) */
 
 void qmp_guest_suspend_disk(Error **err)
@@ -1050,6 +1184,12 @@ GuestNetworkInterfaceList *qmp_guest_network_get_interfaces(Error **errp)
     return NULL;
 }
 
+GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
+{
+    error_set(errp, QERR_UNSUPPORTED);
+    return NULL;
+}
+
 #endif
 
 #if !defined(CONFIG_FSFREEZE)
@@ -1083,12 +1223,6 @@ void qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **err)
 }
 #endif
 
-GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
-{
-    error_set(errp, QERR_UNSUPPORTED);
-    return NULL;
-}
-
 int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
 {
     error_set(errp, QERR_UNSUPPORTED);
-- 
1.7.1

[Qemu-devel] [PATCH v2 3/3] qga: implement qmp_guest_set_vcpus() for Linux with sysfs

[Laszlo Ersek]

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 qga/commands-posix.c |   38 ++++++++++++++++++++++++++++++++------
 1 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index fd38e14..29898a4 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1161,6 +1161,32 @@ GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
     return NULL;
 }
 
+int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
+{
+    int64_t processed;
+    Error *local_err = NULL;
+
+    processed = 0;
+    while (vcpus != NULL) {
+        transfer_vcpu(vcpus->value, false, &local_err);
+        if (local_err != NULL) {
+            break;
+        }
+        ++processed;
+        vcpus = vcpus->next;
+    }
+
+    if (local_err != NULL) {
+        if (processed == 0) {
+            error_propagate(errp, local_err);
+        } else {
+            error_free(local_err);
+        }
+    }
+
+    return processed;
+}
+
 #else /* defined(__linux__) */
 
 void qmp_guest_suspend_disk(Error **err)
@@ -1190,6 +1216,12 @@ GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
     return NULL;
 }
 
+int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
+{
+    error_set(errp, QERR_UNSUPPORTED);
+    return -1;
+}
+
 #endif
 
 #if !defined(CONFIG_FSFREEZE)
@@ -1223,12 +1255,6 @@ void qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **err)
 }
 #endif
 
-int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
-{
-    error_set(errp, QERR_UNSUPPORTED);
-    return -1;
-}
-
 /* register init/cleanup routines for stateful command groups */
 void ga_command_state_init(GAState *s, GACommandState *cs)
 {
-- 
1.7.1

Re: [Qemu-devel] [PATCH v2 1/3] qga: introduce guest-get-vcpus / guest-set-vcpus with stubs

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2977 bytes --]

On 03/06/2013 02:59 PM, Laszlo Ersek wrote:
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  qga/qapi-schema.json |   72 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  qga/commands-posix.c |   12 ++++++++
>  qga/commands-win32.c |   12 ++++++++
>  3 files changed, 96 insertions(+), 0 deletions(-)
> 
> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index d91d903..cba881c 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -515,3 +515,75 @@
>  ##
>  { 'command': 'guest-network-get-interfaces',
>    'returns': ['GuestNetworkInterface'] }
> +
> +##
> +# @GuestLogicalProcessor:
> +#
> +# @logical-id: Arbitrary guest-specific unique identifier of the VCPU.
> +#
> +# @online: Whether the VCPU is enabled.
> +#
> +# @can-offline: Whether offlining the VCPU is possible. This member is always
> +#               filled in by the guest agent when the structure is returned,
> +#               and always ignored on input (hence it can be omitted then).

Other places have used the notation '#optional' when documenting a
parameter that need not be present on input; although we don't have
anything that strictly requires/enforces that notation.

> +#
> +# Since: 1.5
> +##
> +{ 'type': 'GuestLogicalProcessor',
> +  'data': {'logical-id': 'int',

Should logical-id be 'str' instead of 'int', since we said it is
arbitrary what the guest names its vcpus?  Then again, integers can be
made to work (even if the guest OS prefers to name cpus via strings, the
agent can track a 1:1 lookup table between OS string and integer number
handed over qga, perhaps even by returning an invariant pointer address
of the OS string as the integer identifier), so I won't insist.

> +# Returns: The length of the initial sublist that has been successfully
> +#          processed. The guest agent maximizes this value. Possible cases:
> +#
> +#          0:                if the @vcpus list was empty on input. Guest state
> +#                            has not been changed. Otherwise,
> +#
> +#          Error:            processing the first node of @vcpus failed for the
> +#                            reason returned. Guest state has not been changed.
> +#                            Otherwise,
> +#

> +
> +int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
> +{
> +    error_set(errp, QERR_UNSUPPORTED);
> +    return -1;

This returns an error even on an empty input @vcpus, while the docs said
that returning 0 takes priority.  But it's so much of a corner case that
I don't care; always returning an error seems fine.

Thus, although there are things you might change if you have to respin
the series for later review comments, I'm perfectly fine leaving this
as-is and you can use:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH v2 1/3] qga: introduce guest-get-vcpus / guest-set-vcpus with stubs

[Laszlo Ersek]

On 03/06/13 23:32, Eric Blake wrote:
> On 03/06/2013 02:59 PM, Laszlo Ersek wrote:

>> +##
>> +# @GuestLogicalProcessor:
>> +#
>> +# @logical-id: Arbitrary guest-specific unique identifier of the VCPU.
>> +#
>> +# @online: Whether the VCPU is enabled.
>> +#
>> +# @can-offline: Whether offlining the VCPU is possible. This member is always
>> +#               filled in by the guest agent when the structure is returned,
>> +#               and always ignored on input (hence it can be omitted then).
> 
> Other places have used the notation '#optional' when documenting a
> parameter that need not be present on input; although we don't have
> anything that strictly requires/enforces that notation.

I'll fix this in v3 if I'll have to respin, otherwise I'd prefer a
followup patch.

>> +# Returns: The length of the initial sublist that has been successfully
>> +#          processed. The guest agent maximizes this value. Possible cases:
>> +#
>> +#          0:                if the @vcpus list was empty on input. Guest state
>> +#                            has not been changed. Otherwise,
>> +#
>> +#          Error:            processing the first node of @vcpus failed for the
>> +#                            reason returned. Guest state has not been changed.
>> +#                            Otherwise,
>> +#
> 
>> +
>> +int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
>> +{
>> +    error_set(errp, QERR_UNSUPPORTED);
>> +    return -1;
> 
> This returns an error even on an empty input @vcpus, while the docs said
> that returning 0 takes priority.  But it's so much of a corner case that
> I don't care; always returning an error seems fine.

I see what you mean. In my mind, "unsupported" beats everything else, as
if there was a big banner on top of the schema file: "you'll get
QERR_UNSUPPORTED from any interface that's not supported".

I'd like to leave this as-is even if I have to respin; distinguishing
between zero-length-list and "unsupported" seems awkward, plus I'd also
like to accept an empty list without error (in the supported case).

> Thus, although there are things you might change if you have to respin
> the series for later review comments, I'm perfectly fine leaving this
> as-is and you can use:
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks much!
Laszlo

Re: [Qemu-devel] [PATCH v2 2/3] qga: implement qmp_guest_get_vcpus() for Linux with sysfs

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 980 bytes --]

On 03/06/2013 02:59 PM, Laszlo Ersek wrote:
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  qga/commands-posix.c |  146 +++++++++++++++++++++++++++++++++++++++++++++++--
>  1 files changed, 140 insertions(+), 6 deletions(-)
> 

> @@ -1027,6 +1031,136 @@ error:
>      return NULL;
>  }
>  
> +#define SYSCONF_EXACT(name, err) sysconf_exact((name), #name, (err))

Technically, the inner () are redundant (in a parameter list, there is
no syntax that can be rendered differently if you called
sysconf_exact(name, #name, err)); but I don't mind keeping them for
style purposes (the rule of thumb of parenthesizing macro parameters for
safety is a bit easier to remember if you always do it, even when it is
redundant).

> +        vcpu->has_can_offline = true; /* lolspeak ftw */

Indeed :)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH v2 3/3] qga: implement qmp_guest_set_vcpus() for Linux with sysfs

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1228 bytes --]

On 03/06/2013 02:59 PM, Laszlo Ersek wrote:
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  qga/commands-posix.c |   38 ++++++++++++++++++++++++++++++++------
>  1 files changed, 32 insertions(+), 6 deletions(-)
> 
>  
> +int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
> +{
> +    int64_t processed;
> +    Error *local_err = NULL;
> +
> +    processed = 0;
> +    while (vcpus != NULL) {
> +        transfer_vcpu(vcpus->value, false, &local_err);
> +        if (local_err != NULL) {
> +            break;
> +        }
> +        ++processed;
> +        vcpus = vcpus->next;
> +    }
> +
> +    if (local_err != NULL) {
> +        if (processed == 0) {
> +            error_propagate(errp, local_err);

Do we need to set processed to -1 here, to flag to the caller that we
propagated an error?  I'm not sure enough of the mechanics of the call
chain, so maybe this already works even if you leave things as returning 0.

Depending on that answer, you can add:
Reviewed-by: Eric Blake <eblake@redhat.com>
if I didn't find a reason for a respin.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH v2 1/3] qga: introduce guest-get-vcpus / guest-set-vcpus with stubs

[mdroth]

On Wed, Mar 06, 2013 at 11:48:14PM +0100, Laszlo Ersek wrote:
> On 03/06/13 23:32, Eric Blake wrote:
> > On 03/06/2013 02:59 PM, Laszlo Ersek wrote:
> 
> >> +##
> >> +# @GuestLogicalProcessor:
> >> +#
> >> +# @logical-id: Arbitrary guest-specific unique identifier of the VCPU.
> >> +#
> >> +# @online: Whether the VCPU is enabled.
> >> +#
> >> +# @can-offline: Whether offlining the VCPU is possible. This member is always
> >> +#               filled in by the guest agent when the structure is returned,
> >> +#               and always ignored on input (hence it can be omitted then).
> > 
> > Other places have used the notation '#optional' when documenting a
> > parameter that need not be present on input; although we don't have
> > anything that strictly requires/enforces that notation.
> 
> I'll fix this in v3 if I'll have to respin, otherwise I'd prefer a
> followup patch.
> 
> >> +# Returns: The length of the initial sublist that has been successfully
> >> +#          processed. The guest agent maximizes this value. Possible cases:
> >> +#
> >> +#          0:                if the @vcpus list was empty on input. Guest state
> >> +#                            has not been changed. Otherwise,
> >> +#
> >> +#          Error:            processing the first node of @vcpus failed for the
> >> +#                            reason returned. Guest state has not been changed.
> >> +#                            Otherwise,
> >> +#
> > 
> >> +
> >> +int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
> >> +{
> >> +    error_set(errp, QERR_UNSUPPORTED);
> >> +    return -1;
> > 
> > This returns an error even on an empty input @vcpus, while the docs said
> > that returning 0 takes priority.  But it's so much of a corner case that
> > I don't care; always returning an error seems fine.
> 
> I see what you mean. In my mind, "unsupported" beats everything else, as
> if there was a big banner on top of the schema file: "you'll get
> QERR_UNSUPPORTED from any interface that's not supported".
> 
> I'd like to leave this as-is even if I have to respin; distinguishing
> between zero-length-list and "unsupported" seems awkward, plus I'd also
> like to accept an empty list without error (in the supported case).
> 

That's the general understanding for the current interfaces: "unsupported"
is a higher-level error than the errors that individual commands might
document. So I think we should keep this as-is for consistency, and if
it does need to be documented better then a patch adding the big banner
at the top of the schema is probably the best approach actually.

> > Thus, although there are things you might change if you have to respin
> > the series for later review comments, I'm perfectly fine leaving this
> > as-is and you can use:
> > 
> > Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> Thanks much!
> Laszlo
> 

Re: [Qemu-devel] [PATCH v2 2/3] qga: implement qmp_guest_get_vcpus() for Linux with sysfs

[Laszlo Ersek]

On 03/07/13 00:15, Eric Blake wrote:
> On 03/06/2013 02:59 PM, Laszlo Ersek wrote:
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>  qga/commands-posix.c |  146 +++++++++++++++++++++++++++++++++++++++++++++++--
>>  1 files changed, 140 insertions(+), 6 deletions(-)
>>
> 
>> @@ -1027,6 +1031,136 @@ error:
>>      return NULL;
>>  }
>>  
>> +#define SYSCONF_EXACT(name, err) sysconf_exact((name), #name, (err))
> 
> Technically, the inner () are redundant (in a parameter list, there is
> no syntax that can be rendered differently if you called
> sysconf_exact(name, #name, err)); but I don't mind keeping them for
> style purposes (the rule of thumb of parenthesizing macro parameters for
> safety is a bit easier to remember if you always do it, even when it is
> redundant).

You're correct, of course. I was thinking of the comma operator within
the macro argument, but one has to parenthesize that even for the macro
invocation, and then those parens will show up in the replacement text
as well. For example, the following works:

  #define X(a, b) y(a, b)

  static void
  y(int p, int q)
  {
  }

  int
  main(void)
  {
    int t;

    X((t=1, 2), 3);
    return 0;
  }

>> +        vcpu->has_can_offline = true; /* lolspeak ftw */
> 
> Indeed :)

I was hoping you'd appreciate it :)

>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!
Laszlo

Re: [Qemu-devel] [PATCH v2 3/3] qga: implement qmp_guest_set_vcpus() for Linux with sysfs

[Laszlo Ersek]

On 03/07/13 00:20, Eric Blake wrote:
> On 03/06/2013 02:59 PM, Laszlo Ersek wrote:
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>  qga/commands-posix.c |   38 ++++++++++++++++++++++++++++++++------
>>  1 files changed, 32 insertions(+), 6 deletions(-)
>>
>>  
>> +int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
>> +{
>> +    int64_t processed;
>> +    Error *local_err = NULL;
>> +
>> +    processed = 0;
>> +    while (vcpus != NULL) {
>> +        transfer_vcpu(vcpus->value, false, &local_err);
>> +        if (local_err != NULL) {
>> +            break;
>> +        }
>> +        ++processed;
>> +        vcpus = vcpus->next;
>> +    }
>> +
>> +    if (local_err != NULL) {
>> +        if (processed == 0) {
>> +            error_propagate(errp, local_err);
> 
> Do we need to set processed to -1 here, to flag to the caller that we
> propagated an error?  I'm not sure enough of the mechanics of the call
> chain, so maybe this already works even if you leave things as returning 0.

In general,

error set  output value
on output  would appear valid  what to do
---------  ------------------  --------------------------
yes        yes                 error short-circuits value
yes        no                  error short-circuits value
no         yes                 use value
no         no                  should not happen normally, exceptional
                                 cases exist (when it communicates
                                 something different from error=set),
                                 they need documentation

In particular qemu-ga follows suit; from the generated
qmp_marshal_input_guest_set_vcpus() function
[qga/qapi-generated/qga-qmp-marshal.c]:

    retval = qmp_guest_set_vcpus(vcpus, errp);
    if (!error_is_set(errp)) {
        qmp_marshal_output_guest_set_vcpus(retval, ret, errp);
    }

Also, I tested all branches that are reachable without hacking the
kernel or poking into the process with gdb. I fed qga JSON from a
terminal (using
<http://wiki.libvirt.org/page/Qemu_guest_agent#Configure_guest_agent_without_libvirt_interference>
and <http://wiki.qemu.org/Features/QAPI/GuestAgent>), and errors and
retvals are mutually exclusive.

> Depending on that answer, you can add:
> Reviewed-by: Eric Blake <eblake@redhat.com>
> if I didn't find a reason for a respin.

Yay!

Thanks
Laszlo