4:44 a.m.
This patchset does polishing on the QEMU bulk stats implementation.
The main issue this patchset addresses is that, unless a critical
error is found, bulk stats should be silent and ignore errors.
To do so, virResetLastError() is used in a few places, but this
is not enough since errors are logged anyway.
A better approach is to avoid to report error entirely.
The patchset is organized as follows:
- patches 1 to 3 enhances the functions used in the bulk stats
path(s) adding a 'bool report' flag, to let the caller optionally
suppress error reporting.
- patch 4 is a general polishing patch which reduces repetition
of the code in the block stats collection.
Francesco Romani (4):
qemu: make qemuDomainHelperGetVcpus silent
make virNetInterfaceStats silent
qemu: make qemuMonitorGetAllBlockStatsInfo silent
qemu: json monitor: reduce duplicated code
src/lxc/lxc_driver.c | 2 +-
src/openvz/openvz_driver.c | 2 +-
src/qemu/qemu_driver.c | 32 +++++-----
src/qemu/qemu_monitor.c | 12 ++--
src/qemu/qemu_monitor.h | 3 +-
src/qemu/qemu_monitor_json.c | 136 ++++++++++++++++++++-----------------------
src/qemu/qemu_monitor_json.h | 3 +-
src/util/virstats.c | 21 ++++---
src/util/virstats.h | 3 +-
src/xen/xen_hypervisor.c | 2 +-
10 files changed, 111 insertions(+), 105 deletions(-)
--
1.9.3
4:44 a.m.
New subject: [libvirt] [PATCH 1/4] qemu: make qemuDomainHelperGetVcpus silent
The commit 74c066df4d8 introduced an helper to factor a code path
which is shared between the existing API and the new bulk stats API.
In the bulk stats path errors must be silenced unless critical
(e.g. memory allocation failure).
To address this need, this patch adds an argument to disable error reporting.
---
src/qemu/qemu_driver.c | 22 +++++++++++++---------
1 file changed, 13 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index f28082f..dc8d6c3 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1381,8 +1381,10 @@ qemuGetProcessInfo(unsigned long long *cpuTime, int *lastCpu, long
*vm_rss,
static int
-qemuDomainHelperGetVcpus(virDomainObjPtr vm, virVcpuInfoPtr info, int maxinfo,
- unsigned char *cpumaps, int maplen)
+qemuDomainHelperGetVcpus(virDomainObjPtr vm,
+ virVcpuInfoPtr info, int maxinfo,
+ unsigned char *cpumaps, int maplen,
+ bool report)
{
int maxcpu, hostcpus;
size_t i, v;
@@ -1412,8 +1414,10 @@ qemuDomainHelperGetVcpus(virDomainObjPtr vm, virVcpuInfoPtr info,
int maxinfo,
NULL,
vm->pid,
priv->vcpupids[i]) < 0) {
- virReportSystemError(errno, "%s",
- _("cannot get vCPU placement & pCPU
time"));
+ if (report)
+ virReportSystemError(errno, "%s",
+ _("cannot get vCPU placement "
+ "& pCPU time"));
return -1;
}
}
@@ -1440,8 +1444,9 @@ qemuDomainHelperGetVcpus(virDomainObjPtr vm, virVcpuInfoPtr info,
int maxinfo,
virBitmapFree(map);
}
} else {
- virReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("cpu affinity is not
available"));
+ if (report)
+ virReportError(VIR_ERR_OPERATION_INVALID,
+ "%s", _("cpu affinity is not
available"));
return -1;
}
}
@@ -5044,7 +5049,7 @@ qemuDomainGetVcpus(virDomainPtr dom,
goto cleanup;
}
- ret = qemuDomainHelperGetVcpus(vm, info, maxinfo, cpumaps, maplen);
+ ret = qemuDomainHelperGetVcpus(vm, info, maxinfo, cpumaps, maplen, true);
cleanup:
if (vm)
@@ -17530,8 +17535,7 @@ qemuDomainGetStatsVcpu(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
return -1;
if (qemuDomainHelperGetVcpus(dom, cpuinfo, dom->def->vcpus,
- NULL, 0) < 0) {
- virResetLastError();
+ NULL, 0, false) < 0) {
ret = 0; /* it's ok to be silent and go ahead */
goto cleanup;
}
--
1.9.3
5:29 a.m.
New subject: [libvirt] [PATCH 1/4] qemu: make qemuDomainHelperGetVcpus silent
On Fri, Sep 19, 2014 at 11:44:20AM +0200, Francesco Romani wrote:
The commit 74c066df4d8 introduced an helper to factor a code path
which is shared between the existing API and the new bulk stats API.
In the bulk stats path errors must be silenced unless critical
(e.g. memory allocation failure).
To address this need, this patch adds an argument to disable error reporting.
I'm really don't like the approach of adding 'bool reportError' flags to
internal functions as it doesn't scale. These flags end up creeping out
across more & more of the code base and different callers are going to
disagree about which errors should be ignored and which not. Having the
callers use virResetError is preferrable IMHO as it avoids polluting
countless internal functions with ill defined args.
Regards,
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 :|
5:57 a.m.
New subject: [libvirt] [PATCH 1/4] qemu: make qemuDomainHelperGetVcpus silent
On 09/19/14 12:29, Daniel P. Berrange wrote:
On Fri, Sep 19, 2014 at 11:44:20AM +0200, Francesco Romani wrote:
> The commit 74c066df4d8 introduced an helper to factor a code path
> which is shared between the existing API and the new bulk stats API.
> In the bulk stats path errors must be silenced unless critical
> (e.g. memory allocation failure).
>
> To address this need, this patch adds an argument to disable error reporting.
I'm really don't like the approach of adding 'bool reportError' flags to
internal functions as it doesn't scale. These flags end up creeping out
across more & more of the code base and different callers are going to
disagree about which errors should be ignored and which not. Having the
callers use virResetError is preferrable IMHO as it avoids polluting
In Eric's last review he advised to change from virResetError to the
boolean flag so that logs are not polluted. Without this patch, the code
is in the state as you suggest, but it contradicts Eric's stance.
countless internal functions with ill defined args.
Regards,
Daniel
Peter
11:30 a.m.
New subject: [libvirt] [PATCH 1/4] qemu: make qemuDomainHelperGetVcpus silent
On 09/19/2014 04:57 AM, Peter Krempa wrote:
On 09/19/14 12:29, Daniel P. Berrange wrote:
> On Fri, Sep 19, 2014 at 11:44:20AM +0200, Francesco Romani wrote:
>> The commit 74c066df4d8 introduced an helper to factor a code path
>> which is shared between the existing API and the new bulk stats API.
>> In the bulk stats path errors must be silenced unless critical
>> (e.g. memory allocation failure).
>>
>> To address this need, this patch adds an argument to disable error reporting.
>
> I'm really don't like the approach of adding 'bool reportError' flags
to
> internal functions as it doesn't scale. These flags end up creeping out
> across more & more of the code base and different callers are going to
> disagree about which errors should be ignored and which not. Having the
> callers use virResetError is preferrable IMHO as it avoids polluting
In Eric's last review he advised to change from virResetError to the
boolean flag so that logs are not polluted. Without this patch, the code
is in the state as you suggest, but it contradicts Eric's stance.
> countless internal functions with ill defined args.
I think it all boils down to a question of how frequently will these
functions be called, and how frequently will the logs be populated with
messages about an ignored failure. Is this a case where we are better
off waiting until the complaints happen?
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
4:44 a.m.
New subject: [libvirt] [PATCH 2/4] make virNetInterfaceStats silent
virNetInterfaceStats is now used in the bulk stats path, and
in this path errors must be silenced unless critical
(e.g. memory allocation failure).
To address this need, this patch adds an argument to disable error reporting.
---
src/lxc/lxc_driver.c | 2 +-
src/openvz/openvz_driver.c | 2 +-
src/qemu/qemu_driver.c | 6 ++----
src/util/virstats.c | 21 +++++++++++++--------
src/util/virstats.h | 3 ++-
src/xen/xen_hypervisor.c | 2 +-
6 files changed, 20 insertions(+), 16 deletions(-)
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index c3cd62c..2eaeb22 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -3112,7 +3112,7 @@ lxcDomainInterfaceStats(virDomainPtr dom,
}
if (ret == 0)
- ret = virNetInterfaceStats(path, stats);
+ ret = virNetInterfaceStats(path, stats, true);
else
virReportError(VIR_ERR_INVALID_ARG,
_("Invalid path, '%s' is not a known
interface"), path);
diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c
index b62273a..2b39f36 100644
--- a/src/openvz/openvz_driver.c
+++ b/src/openvz/openvz_driver.c
@@ -2010,7 +2010,7 @@ openvzDomainInterfaceStats(virDomainPtr dom,
}
if (ret == 0)
- ret = virNetInterfaceStats(path, stats);
+ ret = virNetInterfaceStats(path, stats, true);
else
virReportError(VIR_ERR_INVALID_ARG,
_("invalid path, '%s' is not a known
interface"), path);
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index dc8d6c3..3ff226f 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -9884,7 +9884,7 @@ qemuDomainInterfaceStats(virDomainPtr dom,
}
if (ret == 0)
- ret = virNetInterfaceStats(path, stats);
+ ret = virNetInterfaceStats(path, stats, true);
else
virReportError(VIR_ERR_INVALID_ARG,
_("invalid path, '%s' is not a known
interface"), path);
@@ -17634,10 +17634,8 @@ qemuDomainGetStatsInterface(virQEMUDriverPtr driver
ATTRIBUTE_UNUSED,
QEMU_ADD_NAME_PARAM(record, maxparams,
"net", i, dom->def->nets[i]->ifname);
- if (virNetInterfaceStats(dom->def->nets[i]->ifname, &tmp) < 0) {
- virResetLastError();
+ if (virNetInterfaceStats(dom->def->nets[i]->ifname, &tmp, false)
< 0)
continue;
- }
QEMU_ADD_NET_PARAM(record, maxparams, i,
"rx.bytes", tmp.rx_bytes);
diff --git a/src/util/virstats.c b/src/util/virstats.c
index c4725ed..496393b 100644
--- a/src/util/virstats.c
+++ b/src/util/virstats.c
@@ -51,7 +51,8 @@
#ifdef __linux__
int
virNetInterfaceStats(const char *path,
- virDomainInterfaceStatsPtr stats)
+ virDomainInterfaceStatsPtr stats,
+ bool report)
{
int path_len;
FILE *fp;
@@ -115,14 +116,16 @@ virNetInterfaceStats(const char *path,
}
VIR_FORCE_FCLOSE(fp);
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("/proc/net/dev: Interface not found"));
+ if (report)
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("/proc/net/dev: Interface not found"));
return -1;
}
#elif defined(HAVE_GETIFADDRS) && defined(AF_LINK)
int
virNetInterfaceStats(const char *path,
- virDomainInterfaceStatsPtr stats)
+ virDomainInterfaceStatsPtr stats,
+ bool report)
{
struct ifaddrs *ifap, *ifa;
struct if_data *ifd;
@@ -158,7 +161,7 @@ virNetInterfaceStats(const char *path,
}
}
- if (ret < 0)
+ if (ret < 0 && report)
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Interface not found"));
@@ -168,10 +171,12 @@ virNetInterfaceStats(const char *path,
#else
int
virNetInterfaceStats(const char *path ATTRIBUTE_UNUSED,
- virDomainInterfaceStatsPtr stats ATTRIBUTE_UNUSED)
+ virDomainInterfaceStatsPtr stats ATTRIBUTE_UNUSED,
+ bool report)
{
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("interface stats not implemented on this platform"));
+ if (report)
+ virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("interface stats not implemented on this platform"));
return -1;
}
diff --git a/src/util/virstats.h b/src/util/virstats.h
index d2c6b64..df993cd 100644
--- a/src/util/virstats.h
+++ b/src/util/virstats.h
@@ -26,6 +26,7 @@
# include "internal.h"
extern int virNetInterfaceStats(const char *path,
- virDomainInterfaceStatsPtr stats);
+ virDomainInterfaceStatsPtr stats,
+ bool report);
#endif /* __STATS_LINUX_H__ */
diff --git a/src/xen/xen_hypervisor.c b/src/xen/xen_hypervisor.c
index d3d4aea..8b257f7 100644
--- a/src/xen/xen_hypervisor.c
+++ b/src/xen/xen_hypervisor.c
@@ -1470,7 +1470,7 @@ xenHypervisorDomainInterfaceStats(virDomainDefPtr def,
return -1;
}
- return virNetInterfaceStats(path, stats);
+ return virNetInterfaceStats(path, stats, true);
#else
virReportError(VIR_ERR_OPERATION_INVALID, "%s",
_("/proc/net/dev: Interface not found"));
--
1.9.3
4:44 a.m.
New subject: [libvirt] [PATCH 3/4] qemu: make qemuMonitorGetAllBlockStatsInfo silent
The commit 290e3c6b07a introduced an helper to factor a code path
which is shared between the existing API and the new bulk stats API.
In the bulk stats path errors must be silenced unless critical
(e.g. memory allocation failure).
To address this need, this patch adds an argument to disable error
reporting.
---
src/qemu/qemu_driver.c | 4 +-
src/qemu/qemu_monitor.c | 12 ++++--
src/qemu/qemu_monitor.h | 3 +-
src/qemu/qemu_monitor_json.c | 87 ++++++++++++++++++++++++++------------------
src/qemu/qemu_monitor_json.h | 3 +-
5 files changed, 65 insertions(+), 44 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 3ff226f..36b96e5 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -17696,12 +17696,12 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
qemuDomainObjEnterMonitor(driver, dom);
nstats = qemuMonitorGetAllBlockStatsInfo(priv->mon, NULL,
- stats, nstats);
+ stats, nstats,
+ false);
qemuDomainObjExitMonitor(driver, dom);
if (nstats < 0) {
- virResetLastError();
ret = 0; /* still ok, again go ahead silently */
goto cleanup;
}
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 10f51c5..cac48d7 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -1765,17 +1765,21 @@ int
qemuMonitorGetAllBlockStatsInfo(qemuMonitorPtr mon,
const char *dev_name,
qemuBlockStatsPtr stats,
- int nstats)
+ int nstats,
+ bool report)
{
int ret;
VIR_DEBUG("mon=%p dev=%s", mon, dev_name);
if (mon->json) {
ret = qemuMonitorJSONGetAllBlockStatsInfo(mon, dev_name,
- stats, nstats);
+ stats, nstats,
+ report);
} else {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("unable to query all block stats with this QEMU"));
+ if (report)
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("unable to query all block stats "
+ "with this QEMU"));
return -1;
}
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 2a43a3c..1eeb7b9 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -363,7 +363,8 @@ struct _qemuBlockStats {
int qemuMonitorGetAllBlockStatsInfo(qemuMonitorPtr mon,
const char *dev_name,
qemuBlockStatsPtr stats,
- int nstats)
+ int nstats,
+ bool report)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3);
int qemuMonitorGetBlockStatsParamsNumber(qemuMonitorPtr mon,
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 857edf1..4810bd3 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -1749,7 +1749,8 @@ int qemuMonitorJSONGetBlockStatsInfo(qemuMonitorPtr mon,
if (flush_total_times)
*flush_total_times = -1;
- if (qemuMonitorJSONGetAllBlockStatsInfo(mon, dev_name, &stats, 1) != 1)
+ if (qemuMonitorJSONGetAllBlockStatsInfo(mon, dev_name,
+ &stats, 1, true) != 1)
goto cleanup;
*rd_req = stats.rd_req;
@@ -1777,7 +1778,8 @@ int qemuMonitorJSONGetBlockStatsInfo(qemuMonitorPtr mon,
int qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitorPtr mon,
const char *dev_name,
qemuBlockStatsPtr bstats,
- int nstats)
+ int nstats,
+ bool report)
{
int ret, count;
size_t i;
@@ -1802,8 +1804,9 @@ int qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitorPtr mon,
devices = virJSONValueObjectGet(reply, "return");
if (!devices || devices->type != VIR_JSON_TYPE_ARRAY) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("blockstats reply was missing device list"));
+ if (report)
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("blockstats reply was missing device list"));
goto cleanup;
}
@@ -1812,9 +1815,10 @@ int qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitorPtr mon,
virJSONValuePtr dev = virJSONValueArrayGet(devices, i);
virJSONValuePtr stats;
if (!dev || dev->type != VIR_JSON_TYPE_OBJECT) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("blockstats device entry was not "
- "in expected format"));
+ if (report)
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("blockstats device entry was not "
+ "in expected format"));
goto cleanup;
}
@@ -1824,9 +1828,10 @@ int qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitorPtr mon,
if (dev_name) {
const char *thisdev = virJSONValueObjectGetString(dev, "device");
if (!thisdev) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("blockstats device entry was not "
- "in expected format"));
+ if (report)
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("blockstats device entry was not "
+ "in expected format"));
goto cleanup;
}
@@ -1843,46 +1848,52 @@ int qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitorPtr mon,
if ((stats = virJSONValueObjectGet(dev, "stats")) == NULL ||
stats->type != VIR_JSON_TYPE_OBJECT) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("blockstats stats entry was not "
- "in expected format"));
+ if (report)
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("blockstats stats entry was not "
+ "in expected format"));
goto cleanup;
}
if (virJSONValueObjectGetNumberLong(stats, "rd_bytes",
&bstats->rd_bytes) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("cannot read %s statistic"),
- "rd_bytes");
+ if (report)
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("cannot read %s statistic"),
+ "rd_bytes");
goto cleanup;
}
if (virJSONValueObjectGetNumberLong(stats, "rd_operations",
&bstats->rd_req) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("cannot read %s statistic"),
- "rd_operations");
+ if (report)
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("cannot read %s statistic"),
+ "rd_operations");
goto cleanup;
}
if (virJSONValueObjectHasKey(stats, "rd_total_time_ns") &&
(virJSONValueObjectGetNumberLong(stats, "rd_total_time_ns",
&bstats->rd_total_times) < 0)) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("cannot read %s statistic"),
- "rd_total_time_ns");
+ if (report)
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("cannot read %s statistic"),
+ "rd_total_time_ns");
goto cleanup;
}
if (virJSONValueObjectGetNumberLong(stats, "wr_bytes",
&bstats->wr_bytes) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("cannot read %s statistic"),
- "wr_bytes");
+ if (report)
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("cannot read %s statistic"),
+ "wr_bytes");
goto cleanup;
}
if (virJSONValueObjectGetNumberLong(stats, "wr_operations",
&bstats->wr_req) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("cannot read %s statistic"),
- "wr_operations");
+ if (report)
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("cannot read %s statistic"),
+ "wr_operations");
goto cleanup;
}
if (virJSONValueObjectHasKey(stats, "wr_total_time_ns") &&
@@ -1896,17 +1907,19 @@ int qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitorPtr mon,
if (virJSONValueObjectHasKey(stats, "flush_operations") &&
(virJSONValueObjectGetNumberLong(stats, "flush_operations",
&bstats->flush_req) < 0)) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("cannot read %s statistic"),
- "flush_operations");
+ if (report)
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("cannot read %s statistic"),
+ "flush_operations");
goto cleanup;
}
if (virJSONValueObjectHasKey(stats, "flush_total_time_ns") &&
(virJSONValueObjectGetNumberLong(stats, "flush_total_time_ns",
&bstats->flush_total_times) < 0))
{
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("cannot read %s statistic"),
- "flush_total_time_ns");
+ if (report)
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("cannot read %s statistic"),
+ "flush_total_time_ns");
goto cleanup;
}
@@ -1918,8 +1931,10 @@ int qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitorPtr mon,
}
if (dev_name && !count) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("cannot find statistics for device '%s'"),
dev_name);
+ if (report)
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("cannot find statistics for device
'%s'"),
+ dev_name);
goto cleanup;
}
diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
index 3daa759..924c79c 100644
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -82,7 +82,8 @@ int qemuMonitorJSONGetBlockStatsInfo(qemuMonitorPtr mon,
int qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitorPtr mon,
const char *dev_name,
qemuBlockStatsPtr stats,
- int nstats);
+ int nstats,
+ bool report);
int qemuMonitorJSONGetBlockStatsParamsNumber(qemuMonitorPtr mon,
int *nparams);
int qemuMonitorJSONGetBlockExtent(qemuMonitorPtr mon,
--
1.9.3
4:44 a.m.
New subject: [libvirt] [PATCH 4/4] qemu: json monitor: reduce duplicated code
This patch replaces repetitive blocks of code with a couple
of macros for the sake of clarity.
There are no changes in behaviour.
---
src/qemu/qemu_monitor_json.c | 129 ++++++++++++++++++-------------------------
1 file changed, 53 insertions(+), 76 deletions(-)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 4810bd3..4fa72c9 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -1775,6 +1775,25 @@ int qemuMonitorJSONGetBlockStatsInfo(qemuMonitorPtr mon,
}
+#define QEMU_MONITOR_JSON_MALFORMED_ENTRY(kind) do { \
+ if (report) \
+ virReportError(VIR_ERR_INTERNAL_ERROR, \
+ _("blockstats %s entry was not " \
+ "in expected format"), \
+ kind); \
+ goto cleanup; \
+} while (0)
+
+
+#define QEMU_MONITOR_JSON_MISSING_STAT(statistic) do { \
+ if (report) \
+ virReportError(VIR_ERR_INTERNAL_ERROR, \
+ _("cannot read %s statistic"), \
+ statistic); \
+ goto cleanup; \
+} while (0)
+
+
int qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitorPtr mon,
const char *dev_name,
qemuBlockStatsPtr bstats,
@@ -1814,26 +1833,16 @@ int qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitorPtr mon,
for (i = 0; i < virJSONValueArraySize(devices) && count < nstats; i++)
{
virJSONValuePtr dev = virJSONValueArrayGet(devices, i);
virJSONValuePtr stats;
- if (!dev || dev->type != VIR_JSON_TYPE_OBJECT) {
- if (report)
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("blockstats device entry was not "
- "in expected format"));
- goto cleanup;
- }
+ if (!dev || dev->type != VIR_JSON_TYPE_OBJECT)
+ QEMU_MONITOR_JSON_MALFORMED_ENTRY("device");
/* If dev_name is specified, we are looking for a specific device,
* so we must be stricter.
*/
if (dev_name) {
const char *thisdev = virJSONValueObjectGetString(dev, "device");
- if (!thisdev) {
- if (report)
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("blockstats device entry was not "
- "in expected format"));
- goto cleanup;
- }
+ if (!thisdev)
+ QEMU_MONITOR_JSON_MALFORMED_ENTRY("device");
/* New QEMU has separate names for host & guest side of the disk
* and libvirt gives the host side a 'drive-' prefix. The passed
@@ -1847,81 +1856,44 @@ int qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitorPtr mon,
}
if ((stats = virJSONValueObjectGet(dev, "stats")) == NULL ||
- stats->type != VIR_JSON_TYPE_OBJECT) {
- if (report)
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("blockstats stats entry was not "
- "in expected format"));
- goto cleanup;
- }
+ stats->type != VIR_JSON_TYPE_OBJECT)
+ QEMU_MONITOR_JSON_MALFORMED_ENTRY("stats");
if (virJSONValueObjectGetNumberLong(stats, "rd_bytes",
- &bstats->rd_bytes) < 0) {
- if (report)
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("cannot read %s statistic"),
- "rd_bytes");
- goto cleanup;
- }
+ &bstats->rd_bytes) < 0)
+ QEMU_MONITOR_JSON_MISSING_STAT("rd_bytes");
+
if (virJSONValueObjectGetNumberLong(stats, "rd_operations",
- &bstats->rd_req) < 0) {
- if (report)
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("cannot read %s statistic"),
- "rd_operations");
- goto cleanup;
- }
+ &bstats->rd_req) < 0)
+ QEMU_MONITOR_JSON_MISSING_STAT("rd_operations");
+
if (virJSONValueObjectHasKey(stats, "rd_total_time_ns") &&
(virJSONValueObjectGetNumberLong(stats, "rd_total_time_ns",
- &bstats->rd_total_times) < 0)) {
- if (report)
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("cannot read %s statistic"),
- "rd_total_time_ns");
- goto cleanup;
- }
+ &bstats->rd_total_times) < 0))
+ QEMU_MONITOR_JSON_MISSING_STAT("rd_total_time_ns");
+
if (virJSONValueObjectGetNumberLong(stats, "wr_bytes",
- &bstats->wr_bytes) < 0) {
- if (report)
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("cannot read %s statistic"),
- "wr_bytes");
- goto cleanup;
- }
+ &bstats->wr_bytes) < 0)
+ QEMU_MONITOR_JSON_MISSING_STAT("wr_bytes");
+
if (virJSONValueObjectGetNumberLong(stats, "wr_operations",
- &bstats->wr_req) < 0) {
- if (report)
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("cannot read %s statistic"),
- "wr_operations");
- goto cleanup;
- }
+ &bstats->wr_req) < 0)
+ QEMU_MONITOR_JSON_MISSING_STAT("wr_operations");
+
if (virJSONValueObjectHasKey(stats, "wr_total_time_ns") &&
(virJSONValueObjectGetNumberLong(stats, "wr_total_time_ns",
- &bstats->wr_total_times) < 0)) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("cannot read %s statistic"),
- "wr_total_time_ns");
- goto cleanup;
- }
+ &bstats->wr_total_times) < 0))
+ QEMU_MONITOR_JSON_MISSING_STAT("wr_total_time_ns");
+
if (virJSONValueObjectHasKey(stats, "flush_operations") &&
(virJSONValueObjectGetNumberLong(stats, "flush_operations",
- &bstats->flush_req) < 0)) {
- if (report)
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("cannot read %s statistic"),
- "flush_operations");
- goto cleanup;
- }
+ &bstats->flush_req) < 0))
+ QEMU_MONITOR_JSON_MISSING_STAT("flush_operations");
+
if (virJSONValueObjectHasKey(stats, "flush_total_time_ns") &&
(virJSONValueObjectGetNumberLong(stats, "flush_total_time_ns",
- &bstats->flush_total_times) < 0))
{
- if (report)
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("cannot read %s statistic"),
- "flush_total_time_ns");
- goto cleanup;
- }
+ &bstats->flush_total_times) < 0))
+ QEMU_MONITOR_JSON_MISSING_STAT("flush_total_time_ns");
count++;
bstats++;
@@ -1947,6 +1919,11 @@ int qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitorPtr mon,
}
+#undef QEMU_MONITOR_JSON_MISSING_STAT
+
+#undef QEMU_MONITOR_JSON_MALFORMED_ENTRY
+
+
int qemuMonitorJSONGetBlockStatsParamsNumber(qemuMonitorPtr mon,
int *nparams)
{
--
1.9.3
3750
days inactive
3753
days old
7 comments
4 participants
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Francesco Romani -
Peter Krempa