| Message ID | 1362607171-24668-4-git-send-email-lersek@redhat.com |
|---|---|
| State | New |
| Headers | show |
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.
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
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) {
Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- qga/commands-posix.c | 38 ++++++++++++++++++++++++++++++++------ 1 files changed, 32 insertions(+), 6 deletions(-)