Patchwork [v2,3/3] qga: implement qmp_guest_set_vcpus() for Linux with sysfs

login
register
mail settings
Submitter Laszlo Ersek
Date March 6, 2013, 9:59 p.m.
Message ID <1362607171-24668-4-git-send-email-lersek@redhat.com>
Download mbox | patch
Permalink /patch/225657/
State New
Headers show

Comments

Laszlo Ersek - March 6, 2013, 9:59 p.m.
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 qga/commands-posix.c |   38 ++++++++++++++++++++++++++++++++------
 1 files changed, 32 insertions(+), 6 deletions(-)
Eric Blake - March 6, 2013, 11:20 p.m.
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.
Laszlo Ersek - March 6, 2013, 11:55 p.m.
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

Patch

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)
 {