[libvirt] [PATCH] virterror: Add error message for unsupported operations.
This patch introduces a new error code VIR_ERR_OPERATION_UNSUPPORTED to mark error messages regarding operations that failed due to lack of support on the hypervisor or other than libvirt issues. The code is first used in reporting error if qemu does not support block IO tuning variables yielding error message: error: Unable to get block I/O throttle parameters error: Operation not supported: block_io_throttle field 'total_bytes_sec' missing in qemu's output instead of: error: Unable to get block I/O throttle parameters error: internal error cannot read total_bytes_sec --- include/libvirt/virterror.h | 2 ++ src/qemu/qemu_monitor_json.c | 4 ++-- src/util/virterror.c | 6 ++++++ 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h index ad8e101..913fc5d 100644 --- a/include/libvirt/virterror.h +++ b/include/libvirt/virterror.h @@ -277,6 +277,8 @@ typedef enum { VIR_ERR_MIGRATE_UNSAFE = 81, /* Migration is not safe */ VIR_ERR_OVERFLOW = 82, /* integer overflow */ VIR_ERR_BLOCK_COPY_ACTIVE = 83, /* action prevented by block copy job */ + VIR_ERR_OPERATION_UNSUPPORTED = 84, /* The requested operation is not + supported */ } virErrorNumber; /** diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 3ede88d..7e482d1 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -3652,11 +3652,11 @@ int qemuMonitorJSONOpenGraphics(qemuMonitorPtr mon, if (virJSONValueObjectGetNumberUlong(inserted, \ FIELD, \ &reply->STORE) < 0) { \ - virReportError(VIR_ERR_INTERNAL_ERROR, \ + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, \ _("block_io_throttle field '%s' missing " \ "in qemu's output"), \ #STORE); \ - goto cleanup; \ + goto cleanup; \ } static int qemuMonitorJSONBlockIoThrottleInfo(virJSONValuePtr result, diff --git a/src/util/virterror.c b/src/util/virterror.c index a40cfe0..c438de8 100644 --- a/src/util/virterror.c +++ b/src/util/virterror.c @@ -1185,6 +1185,12 @@ virErrorMsg(virErrorNumber error, const char *info) else errmsg = _("block copy still active: %s"); break; + case VIR_ERR_OPERATION_UNSUPPORTED: + if (!info) + errmsg = _("Operation not supported"); + else + errmsg = _("Operation not supported: %s"); + break; } return errmsg; } -- 1.7.8.6
On 08/09/2012 07:31 AM, Peter Krempa wrote:
This patch introduces a new error code VIR_ERR_OPERATION_UNSUPPORTED to mark error messages regarding operations that failed due to lack of support on the hypervisor or other than libvirt issues.
The code is first used in reporting error if qemu does not support block IO tuning variables yielding error message: error: Unable to get block I/O throttle parameters error: Operation not supported: block_io_throttle field 'total_bytes_sec' missing in qemu's output
instead of: error: Unable to get block I/O throttle parameters error: internal error cannot read total_bytes_sec ---
In the past, we have used VIR_ERR_CONFIG_UNSUPPORTED for messages about a qemu binary that doesn't support something; would that be any better than inventing a new error here? Or are all of those errors worth switching over to this new code? As written, your patch seems fine, but only if we agree that a new error is the way to go. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 08/09/12 15:38, Eric Blake wrote:
On 08/09/2012 07:31 AM, Peter Krempa wrote:
This patch introduces a new error code VIR_ERR_OPERATION_UNSUPPORTED to mark error messages regarding operations that failed due to lack of support on the hypervisor or other than libvirt issues.
The code is first used in reporting error if qemu does not support block IO tuning variables yielding error message: error: Unable to get block I/O throttle parameters error: Operation not supported: block_io_throttle field 'total_bytes_sec' missing in qemu's output
instead of: error: Unable to get block I/O throttle parameters error: internal error cannot read total_bytes_sec ---
In the past, we have used VIR_ERR_CONFIG_UNSUPPORTED for messages about a qemu binary that doesn't support something; would that be any better than inventing a new error here? Or are all of those errors worth switching over to this new code?
Using VIR_ERR_CONFIG_UNSUPPORTED seems reasonable to me in cases where the unsupported feature is requested by the user. Eg. when setting something or requesting some weird configuration or even if the hypervisor doesn't support it. On the other hand It doesn't make sense to me to use it on getter-type APIs where the user isn't setting anything just wants some information back. In this case I'd rather like to see a new message, as stating that config isn't supported is a little bit strange.
As written, your patch seems fine, but only if we agree that a new error is the way to go.
Thanks for reviewing.
Peter
On 08/09/2012 09:44 AM, Peter Krempa wrote:
On 08/09/12 15:38, Eric Blake wrote:
On 08/09/2012 07:31 AM, Peter Krempa wrote:
This patch introduces a new error code VIR_ERR_OPERATION_UNSUPPORTED to mark error messages regarding operations that failed due to lack of support on the hypervisor or other than libvirt issues.
The code is first used in reporting error if qemu does not support block IO tuning variables yielding error message: error: Unable to get block I/O throttle parameters error: Operation not supported: block_io_throttle field 'total_bytes_sec' missing in qemu's output
instead of: error: Unable to get block I/O throttle parameters error: internal error cannot read total_bytes_sec ---
In the past, we have used VIR_ERR_CONFIG_UNSUPPORTED for messages about a qemu binary that doesn't support something; would that be any better than inventing a new error here? Or are all of those errors worth switching over to this new code?
Using VIR_ERR_CONFIG_UNSUPPORTED seems reasonable to me in cases where the unsupported feature is requested by the user. Eg. when setting something or requesting some weird configuration or even if the hypervisor doesn't support it.
On the other hand It doesn't make sense to me to use it on getter-type APIs where the user isn't setting anything just wants some information back. In this case I'd rather like to see a new message, as stating that config isn't supported is a little bit strange.
As written, your patch seems fine, but only if we agree that a new error is the way to go.
There was already a short discussion about this related to another patch: https://www.redhat.com/archives/libvir-list/2012-August/msg00589.html Peter wrote this patch in response to that discussion. My vote is in favor of the new code. <knows-a-bandwagon-when-he-sees-it/>
On 08/09/2012 11:34 PM, Laine Stump wrote:
On 08/09/2012 09:44 AM, Peter Krempa wrote:
On 08/09/12 15:38, Eric Blake wrote:
On 08/09/2012 07:31 AM, Peter Krempa wrote:
This patch introduces a new error code VIR_ERR_OPERATION_UNSUPPORTED to mark error messages regarding operations that failed due to lack of support on the hypervisor or other than libvirt issues.
In the past, we have used VIR_ERR_CONFIG_UNSUPPORTED for messages about a qemu binary that doesn't support something; would that be any better than inventing a new error here? Or are all of those errors worth switching over to this new code?
Using VIR_ERR_CONFIG_UNSUPPORTED seems reasonable to me in cases where the unsupported feature is requested by the user. Eg. when setting something or requesting some weird configuration or even if the hypervisor doesn't support it.
On the other hand It doesn't make sense to me to use it on getter-type APIs where the user isn't setting anything just wants some information back. In this case I'd rather like to see a new message, as stating that config isn't supported is a little bit strange.
Makes sense to me - the user is not trying to change config, but is merely being informed that the information they requested is not available.
As written, your patch seems fine, but only if we agree that a new error is the way to go.
There was already a short discussion about this related to another patch:
https://www.redhat.com/archives/libvir-list/2012-August/msg00589.html
Peter wrote this patch in response to that discussion.
My vote is in favor of the new code.
Sounds like we've got enough arguments in favor of the new code; go ahead and push the new error type. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (3)
-
Eric Blake -
Laine Stump -
Peter Krempa