[libvirt] [PATCH] qemu: Refactor parsing of block device IO tuning parameters.
This patch refactors the JSON parsing function that extracts the block IO tuning parameters from qemu's output. The most impacting change concerns the error message that is returned if the reply from qemu does not contain the needed data. The data for IO parameter tuning were added in qemu 1.1 and the previous error message was confusing. This patch also breaks long lines and extracts a multiple time used code pattern to a macro. --- Old error message looks like: # virsh blkdeviotune asdf hda error: Unable to get block I/O throttle parameters error: internal error cannot read total_bytes_sec and the new: # virsh blkdeviotune asdf hda error: Unable to get block I/O throttle parameters error: internal error block_io_throttle field 'total_bytes_sec' missing in qemu's output src/qemu/qemu_monitor_json.c | 66 ++++++++++++++++------------------------- 1 files changed, 26 insertions(+), 40 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index dd6536f..3ede88d 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -3648,6 +3648,16 @@ int qemuMonitorJSONOpenGraphics(qemuMonitorPtr mon, } +#define GET_THROTTLE_STATS(FIELD, STORE) \ + if (virJSONValueObjectGetNumberUlong(inserted, \ + FIELD, \ + &reply->STORE) < 0) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, \ + _("block_io_throttle field '%s' missing " \ + "in qemu's output"), \ + #STORE); \ + goto cleanup; \ + } static int qemuMonitorJSONBlockIoThrottleInfo(virJSONValuePtr result, const char *device, @@ -3656,7 +3666,7 @@ qemuMonitorJSONBlockIoThrottleInfo(virJSONValuePtr result, virJSONValuePtr io_throttle; int ret = -1; int i; - int found = 0; + bool found = false; io_throttle = virJSONValueObjectGet(result, "return"); @@ -3673,13 +3683,15 @@ qemuMonitorJSONBlockIoThrottleInfo(virJSONValuePtr result, if (!temp_dev || temp_dev->type != VIR_JSON_TYPE_OBJECT) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("block_io_throttle device entry was not in expected format")); + _("block_io_throttle device entry " + "was not in expected format")); goto cleanup; } - if ((current_dev = virJSONValueObjectGetString(temp_dev, "device")) == NULL) { + if (!(current_dev = virJSONValueObjectGetString(temp_dev, "device"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("block_io_throttle device entry was not in expected format")); + _("block_io_throttle device entry " + "was not in expected format")); goto cleanup; } @@ -3689,49 +3701,22 @@ qemuMonitorJSONBlockIoThrottleInfo(virJSONValuePtr result, if (STREQ(current_dev, device)) continue; - found = 1; + found = true; if ((inserted = virJSONValueObjectGet(temp_dev, "inserted")) == NULL || inserted->type != VIR_JSON_TYPE_OBJECT) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("block_io_throttle inserted entry was not in expected format")); + _("block_io_throttle inserted entry " + "was not in expected format")); goto cleanup; } - if (virJSONValueObjectGetNumberUlong(inserted, "bps", &reply->total_bytes_sec) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("cannot read total_bytes_sec")); - goto cleanup; - } + GET_THROTTLE_STATS("bps", total_bytes_sec); + GET_THROTTLE_STATS("bps_rd", read_bytes_sec); + GET_THROTTLE_STATS("bps_wr", write_bytes_sec); + GET_THROTTLE_STATS("iops", total_iops_sec); + GET_THROTTLE_STATS("iops_rd", read_iops_sec); + GET_THROTTLE_STATS("iops_wr", write_iops_sec); - if (virJSONValueObjectGetNumberUlong(inserted, "bps_rd", &reply->read_bytes_sec) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("cannot read read_bytes_sec")); - goto cleanup; - } - - if (virJSONValueObjectGetNumberUlong(inserted, "bps_wr", &reply->write_bytes_sec) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("cannot read write_bytes_sec")); - goto cleanup; - } - - if (virJSONValueObjectGetNumberUlong(inserted, "iops", &reply->total_iops_sec) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("cannot read total_iops_sec")); - goto cleanup; - } - - if (virJSONValueObjectGetNumberUlong(inserted, "iops_rd", &reply->read_iops_sec) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("cannot read read_iops_sec")); - goto cleanup; - } - - if (virJSONValueObjectGetNumberUlong(inserted, "iops_wr", &reply->write_iops_sec) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("cannot read write_iops_sec")); - goto cleanup; - } break; } @@ -3746,6 +3731,7 @@ qemuMonitorJSONBlockIoThrottleInfo(virJSONValuePtr result, cleanup: return ret; } +#undef GET_THROTTLE_STATS int qemuMonitorJSONSetBlockIoThrottle(qemuMonitorPtr mon, const char *device, -- 1.7.8.6
On Thu, Aug 09, 2012 at 10:48:42 +0200, Peter Krempa wrote:
This patch refactors the JSON parsing function that extracts the block IO tuning parameters from qemu's output. The most impacting change concerns the error message that is returned if the reply from qemu does not contain the needed data. The data for IO parameter tuning were added in qemu 1.1 and the previous error message was confusing.
This patch also breaks long lines and extracts a multiple time used code pattern to a macro. --- Old error message looks like: # virsh blkdeviotune asdf hda error: Unable to get block I/O throttle parameters error: internal error cannot read total_bytes_sec
and the new: # virsh blkdeviotune asdf hda error: Unable to get block I/O throttle parameters error: internal error block_io_throttle field 'total_bytes_sec' missing in qemu's output
I think the old/new error messages can be included directly in the commit message. Anyway, I wonder if we should take this as an opportunity to fix the "internal error", however I'm not sure what is the best code to use. It seems we use OPERATION_INVALID in such cases, which could be good enough. ACK Jirka
On Thu, Aug 09, 2012 at 11:39:57AM +0200, Jiri Denemark wrote:
On Thu, Aug 09, 2012 at 10:48:42 +0200, Peter Krempa wrote:
This patch refactors the JSON parsing function that extracts the block IO tuning parameters from qemu's output. The most impacting change concerns the error message that is returned if the reply from qemu does not contain the needed data. The data for IO parameter tuning were added in qemu 1.1 and the previous error message was confusing.
This patch also breaks long lines and extracts a multiple time used code pattern to a macro. --- Old error message looks like: # virsh blkdeviotune asdf hda error: Unable to get block I/O throttle parameters error: internal error cannot read total_bytes_sec
and the new: # virsh blkdeviotune asdf hda error: Unable to get block I/O throttle parameters error: internal error block_io_throttle field 'total_bytes_sec' missing in qemu's output
I think the old/new error messages can be included directly in the commit message. Anyway, I wonder if we should take this as an opportunity to fix the "internal error", however I'm not sure what is the best code to use. It seems we use OPERATION_INVALID in such cases, which could be good enough.
No, OPERATION_INVALID is for scenarios where the API request cannot be performed because the object is in the wrong state. eg when you request to pause a VM that is shutoff. This is a case where the data we receive back from QEMU is incorrect. I don't think we have a current error code that is suitable for this kind of scenario, so we ought to invent a new one. Some ideas MISSING_DATA DATA_INVALID UNEXPECTED_DATA ...insert others... my favour is probably DATA_INVALID Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Thu, Aug 09, 2012 at 10:48:43 +0100, Daniel P. Berrange wrote:
On Thu, Aug 09, 2012 at 11:39:57AM +0200, Jiri Denemark wrote:
On Thu, Aug 09, 2012 at 10:48:42 +0200, Peter Krempa wrote:
# virsh blkdeviotune asdf hda error: Unable to get block I/O throttle parameters error: internal error block_io_throttle field 'total_bytes_sec' missing in qemu's output
I think the old/new error messages can be included directly in the commit message. Anyway, I wonder if we should take this as an opportunity to fix the "internal error", however I'm not sure what is the best code to use. It seems we use OPERATION_INVALID in such cases, which could be good enough.
No, OPERATION_INVALID is for scenarios where the API request cannot be performed because the object is in the wrong state.
eg when you request to pause a VM that is shutoff.
This is a case where the data we receive back from QEMU is incorrect. I don't think we have a current error code that is suitable for this kind of scenario, so we ought to invent a new one.
Some ideas
MISSING_DATA DATA_INVALID UNEXPECTED_DATA ...insert others...
my favour is probably DATA_INVALID
The data is actually correct, just the fields required for this functionality are missing because QEMU is too old to support them. Perhaps OPERATION_UNSUPPORTED (which doesn't currently exist either) could be the right choice. Jirka
On Thu, Aug 09, 2012 at 12:52:16PM +0200, Jiri Denemark wrote:
On Thu, Aug 09, 2012 at 10:48:43 +0100, Daniel P. Berrange wrote:
On Thu, Aug 09, 2012 at 11:39:57AM +0200, Jiri Denemark wrote:
On Thu, Aug 09, 2012 at 10:48:42 +0200, Peter Krempa wrote:
# virsh blkdeviotune asdf hda error: Unable to get block I/O throttle parameters error: internal error block_io_throttle field 'total_bytes_sec' missing in qemu's output
I think the old/new error messages can be included directly in the commit message. Anyway, I wonder if we should take this as an opportunity to fix the "internal error", however I'm not sure what is the best code to use. It seems we use OPERATION_INVALID in such cases, which could be good enough.
No, OPERATION_INVALID is for scenarios where the API request cannot be performed because the object is in the wrong state.
eg when you request to pause a VM that is shutoff.
This is a case where the data we receive back from QEMU is incorrect. I don't think we have a current error code that is suitable for this kind of scenario, so we ought to invent a new one.
Some ideas
MISSING_DATA DATA_INVALID UNEXPECTED_DATA ...insert others...
my favour is probably DATA_INVALID
The data is actually correct, just the fields required for this functionality are missing because QEMU is too old to support them. Perhaps OPERATION_UNSUPPORTED (which doesn't currently exist either) could be the right choice.
We do have a VIR_ERR_NO_SUPPORT, but its probably wise to restrict that to only the case where the public API is not provided by the current driver. So agree that a new OPERATION_UNSUPPORTED would be reasonable Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 08/09/12 13:00, Daniel P. Berrange wrote:
On Thu, Aug 09, 2012 at 12:52:16PM +0200, Jiri Denemark wrote:
On Thu, Aug 09, 2012 at 10:48:43 +0100, Daniel P. Berrange wrote:
On Thu, Aug 09, 2012 at 11:39:57AM +0200, Jiri Denemark wrote:
On Thu, Aug 09, 2012 at 10:48:42 +0200, Peter Krempa wrote:
# virsh blkdeviotune asdf hda error: Unable to get block I/O throttle parameters error: internal error block_io_throttle field 'total_bytes_sec' missing in qemu's output
I think the old/new error messages can be included directly in the commit message. Anyway, I wonder if we should take this as an opportunity to fix the "internal error", however I'm not sure what is the best code to use. It seems we use OPERATION_INVALID in such cases, which could be good enough.
No, OPERATION_INVALID is for scenarios where the API request cannot be performed because the object is in the wrong state.
eg when you request to pause a VM that is shutoff.
This is a case where the data we receive back from QEMU is incorrect. I don't think we have a current error code that is suitable for this kind of scenario, so we ought to invent a new one.
Some ideas
MISSING_DATA DATA_INVALID UNEXPECTED_DATA ...insert others...
my favour is probably DATA_INVALID
The data is actually correct, just the fields required for this functionality are missing because QEMU is too old to support them. Perhaps OPERATION_UNSUPPORTED (which doesn't currently exist either) could be the right choice.
We do have a VIR_ERR_NO_SUPPORT, but its probably wise to restrict that to only the case where the public API is not provided by the current driver. So agree that a new OPERATION_UNSUPPORTED would be reasonable
Daniel
I've pushed this patch and I'll follow up with a separate one adding VIR_ERR_OPERATION_UNSUPPORTED and changing the error message type in that particular case. Peter
participants (3)
-
Daniel P. Berrange -
Jiri Denemark -
Peter Krempa