[libvirt] [PATCH] qemu: Exit on first error in qemuDomainGetMemoryParameters
There is no point in trying to fill params beyond the first error, because when qemuDomainGetMemoryParameters returns -1 then the caller cannot detect which values in params are valid. --- There is a similar pattern in qemuDomainSetMemoryParameters that tries to apply all given params even if one already failed. From a user's POV it's probably better to apply all params without an error or to apply non at all when one fails. But this is harder to implemeneted and requires a rollback mechanism. src/qemu/qemu_driver.c | 21 ++++++++------------- 1 files changed, 8 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6395c93..ae1d833 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9676,7 +9676,6 @@ static int qemuDomainGetMemoryParameters(virDomainPtr dom, goto cleanup; } - ret = 0; for (i = 0; i < *nparams; i++) { virMemoryParameterPtr param = ¶ms[i]; val = 0; @@ -9689,14 +9688,12 @@ static int qemuDomainGetMemoryParameters(virDomainPtr dom, if (rc != 0) { virReportSystemError(-rc, "%s", _("unable to get memory hard limit")); - ret = -1; - continue; + goto cleanup; } if (virStrcpyStatic(param->field, VIR_DOMAIN_MEMORY_HARD_LIMIT) == NULL) { qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Field memory hard limit too long for destination")); - ret = -1; - continue; + goto cleanup; } param->value.ul = val; break; @@ -9706,14 +9703,12 @@ static int qemuDomainGetMemoryParameters(virDomainPtr dom, if (rc != 0) { virReportSystemError(-rc, "%s", _("unable to get memory soft limit")); - ret = -1; - continue; + goto cleanup; } if (virStrcpyStatic(param->field, VIR_DOMAIN_MEMORY_SOFT_LIMIT) == NULL) { qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Field memory soft limit too long for destination")); - ret = -1; - continue; + goto cleanup; } param->value.ul = val; break; @@ -9723,14 +9718,12 @@ static int qemuDomainGetMemoryParameters(virDomainPtr dom, if (rc != 0) { virReportSystemError(-rc, "%s", _("unable to get swap hard limit")); - ret = -1; - continue; + goto cleanup; } if (virStrcpyStatic(param->field, VIR_DOMAIN_MEMORY_SWAP_HARD_LIMIT) == NULL) { qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Field swap hard limit too long for destination")); - ret = -1; - continue; + goto cleanup; } param->value.ul = val; break; @@ -9741,6 +9734,8 @@ static int qemuDomainGetMemoryParameters(virDomainPtr dom, } } + ret = 0; + cleanup: if (group) virCgroupFree(&group); -- 1.7.0.4
On Wed, Oct 20, 2010 at 02:37:53PM +0200, Matthias Bolte wrote:
There is no point in trying to fill params beyond the first error, because when qemuDomainGetMemoryParameters returns -1 then the caller cannot detect which values in params are valid. ---
Okay ACK
There is a similar pattern in qemuDomainSetMemoryParameters that tries to apply all given params even if one already failed. From a user's POV it's probably better to apply all params without an error or to apply non at all when one fails. But this is harder to implemeneted and requires a rollback mechanism.
Well I don't think we can implement transactions there, so there will always a case for uncertainties. Still since the client may want to tune his guest domain, maybe some of the tuning will fail (and with the current API, only retrying with one setting at a time can allow to isolate the problem), but after the call we get closer to the expected setup. It's still possible to see what was done with the Get API though. The simplest way from a client perspective is probably to change only one setting at a time, it's also likely to match the UI offered to the end user. Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
2010/10/20 Daniel Veillard <veillard@redhat.com>:
On Wed, Oct 20, 2010 at 02:37:53PM +0200, Matthias Bolte wrote:
There is no point in trying to fill params beyond the first error, because when qemuDomainGetMemoryParameters returns -1 then the caller cannot detect which values in params are valid. ---
Okay ACK
Thanks, pushed.
There is a similar pattern in qemuDomainSetMemoryParameters that tries to apply all given params even if one already failed. From a user's POV it's probably better to apply all params without an error or to apply non at all when one fails. But this is harder to implemeneted and requires a rollback mechanism.
Well I don't think we can implement transactions there, so there will always a case for uncertainties. Still since the client may want to tune his guest domain, maybe some of the tuning will fail (and with the current API, only retrying with one setting at a time can allow to isolate the problem), but after the call we get closer to the expected setup. It's still possible to see what was done with the Get API though. The simplest way from a client perspective is probably to change only one setting at a time, it's also likely to match the UI offered to the end user.
Daniel
Okay, that makes sense. Matthias
participants (2)
-
Daniel Veillard -
Matthias Bolte