12:15 p.m.
There are other functions that can be used instead so avoid duplication.
Peter Krempa (5):
internal: Introduce virCheckNonEmptyStringArgGoto and reuse it
qemu: monitor: Fix indentation in qemuMonitorJSONGetOneBlockStatsInfo
qemu: monitor: Open-code retrieval of wr_highest_offset
qemu: Refactor qemuDomainGetBlockInfo
qemu: monitor: Remove qemuMonitorGetBlockExtent
src/internal.h | 11 +++
src/libvirt-domain.c | 4 +-
src/qemu/qemu_driver.c | 101 +++++++++++++-------------
src/qemu/qemu_monitor.c | 16 -----
src/qemu/qemu_monitor.h | 4 +-
src/qemu/qemu_monitor_json.c | 164 +++++--------------------------------------
src/qemu/qemu_monitor_json.h | 3 -
src/qemu/qemu_monitor_text.c | 10 ---
src/qemu/qemu_monitor_text.h | 3 -
src/util/virerror.h | 11 +++
tests/qemumonitorjsontest.c | 54 ++++----------
tests/qemumonitortest.c | 10 +--
12 files changed, 114 insertions(+), 277 deletions(-)
--
2.4.1
12:15 p.m.
New subject: [libvirt] [PATCH 1/5] internal: Introduce virCheckNonEmptyStringArgGoto and reuse it
The helper makes sure that strings passed to APIs are non-NULL and
non-empty. This allows to drop some inlined checks where it does not
make sense.
---
src/internal.h | 11 +++++++++++
src/libvirt-domain.c | 4 ++--
src/qemu/qemu_driver.c | 11 -----------
src/util/virerror.h | 11 +++++++++++
4 files changed, 24 insertions(+), 13 deletions(-)
diff --git a/src/internal.h b/src/internal.h
index 7c042e0..db26fb0 100644
--- a/src/internal.h
+++ b/src/internal.h
@@ -446,6 +446,17 @@
goto label; \
} \
} while (0)
+# define virCheckNonEmptyStringArgGoto(argname, label) \
+ do { \
+ if (argname == NULL) { \
+ virReportInvalidNonNullArg(argname); \
+ goto label; \
+ } \
+ if (*argname == '\0') { \
+ virReportInvalidEmptyStringArg(argname); \
+ goto label; \
+ } \
+ } while (0)
# define virCheckPositiveArgGoto(argname, label) \
do { \
if (argname <= 0) { \
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 4d7b88a..909c264 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -6065,7 +6065,7 @@ virDomainBlockPeek(virDomainPtr dom,
conn = dom->conn;
virCheckReadOnlyGoto(conn->flags, error);
- virCheckNonNullArgGoto(disk, error);
+ virCheckNonEmptyStringArgGoto(disk, error);
/* Allow size == 0 as an access test. */
if (size > 0)
@@ -6333,7 +6333,7 @@ virDomainGetBlockInfo(virDomainPtr domain, const char *disk,
memset(info, 0, sizeof(*info));
virCheckDomainReturn(domain, -1);
- virCheckNonNullArgGoto(disk, error);
+ virCheckNonEmptyStringArgGoto(disk, error);
virCheckNonNullArgGoto(info, error);
conn = domain->conn;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index c1373de..004da7e 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -11530,12 +11530,6 @@ qemuDomainBlockPeek(virDomainPtr dom,
if (virDomainBlockPeekEnsureACL(dom->conn, vm->def) < 0)
goto cleanup;
- if (!path || path[0] == '\0') {
- virReportError(VIR_ERR_INVALID_ARG,
- "%s", _("NULL or empty path"));
- goto cleanup;
- }
-
/* Check the path belongs to this domain. */
if (!(actual = virDomainDiskPathByName(vm->def, path))) {
virReportError(VIR_ERR_INVALID_ARG,
@@ -11821,11 +11815,6 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
if (virDomainGetBlockInfoEnsureACL(dom->conn, vm->def) < 0)
goto cleanup;
- if (!path || path[0] == '\0') {
- virReportError(VIR_ERR_INVALID_ARG, "%s", _("NULL or empty
path"));
- goto cleanup;
- }
-
/* Technically, we only need a job if we are going to query the
* monitor, which is only for active domains that are using
* non-raw block devices. But it is easier to share code if we
diff --git a/src/util/virerror.h b/src/util/virerror.h
index ad3a946..c1a445e 100644
--- a/src/util/virerror.h
+++ b/src/util/virerror.h
@@ -95,6 +95,17 @@ void virReportSystemErrorFull(int domcode,
0, 0, \
_("%s in %s must not be NULL"), \
#argname, __FUNCTION__)
+# define virReportInvalidEmptyStringArg(argname) \
+ virRaiseErrorFull(__FILE__, __FUNCTION__, __LINE__, \
+ VIR_FROM_THIS, \
+ VIR_ERR_INVALID_ARG, \
+ VIR_ERR_ERROR, \
+ __FUNCTION__, \
+ #argname, \
+ NULL, \
+ 0, 0, \
+ _("string %s in %s must not be non empty"), \
+ #argname, __FUNCTION__)
# define virReportInvalidPositiveArg(argname) \
virRaiseErrorFull(__FILE__, __FUNCTION__, __LINE__, \
VIR_FROM_THIS, \
--
2.4.1
11:23 a.m.
New subject: [libvirt] [PATCH 1/5] internal: Introduce virCheckNonEmptyStringArgGoto and reuse it
On 06/23/2015 01:15 PM, Peter Krempa wrote:
The helper makes sure that strings passed to APIs are non-NULL and
non-empty. This allows to drop some inlined checks where it does not
make sense.
---
src/internal.h | 11 +++++++++++
src/libvirt-domain.c | 4 ++--
src/qemu/qemu_driver.c | 11 -----------
src/util/virerror.h | 11 +++++++++++
4 files changed, 24 insertions(+), 13 deletions(-)
...
diff --git a/src/util/virerror.h b/src/util/virerror.h
index ad3a946..c1a445e 100644
--- a/src/util/virerror.h
+++ b/src/util/virerror.h
@@ -95,6 +95,17 @@ void virReportSystemErrorFull(int domcode,
0, 0, \
_("%s in %s must not be NULL"), \
#argname, __FUNCTION__)
+# define virReportInvalidEmptyStringArg(argname) \
+ virRaiseErrorFull(__FILE__, __FUNCTION__, __LINE__, \
+ VIR_FROM_THIS, \
+ VIR_ERR_INVALID_ARG, \
+ VIR_ERR_ERROR, \
+ __FUNCTION__, \
+ #argname, \
+ NULL, \
+ 0, 0, \
+ _("string %s in %s must not be non empty"), \
This reads strangely... The double negative (not and non)
"string %s in %s must not be empty" or "string %s in %s must be non
empty"
Or more technically
"string %s in %s cannot start with the NUL character" (yes, correct
spelling for NUL - that's the '\0')
+ #argname, __FUNCTION__)
# define virReportInvalidPositiveArg(argname) \
virRaiseErrorFull(__FILE__, __FUNCTION__, __LINE__, \
VIR_FROM_THIS, \
ACK with some sort of change
John
1:30 a.m.
New subject: [libvirt] [PATCH 1/5] internal: Introduce virCheckNonEmptyStringArgGoto and reuse it
On Thu, Jun 25, 2015 at 12:23:43 -0400, John Ferlan wrote:
On 06/23/2015 01:15 PM, Peter Krempa wrote:
> The helper makes sure that strings passed to APIs are non-NULL and
> non-empty. This allows to drop some inlined checks where it does not
> make sense.
> ---
> src/internal.h | 11 +++++++++++
> src/libvirt-domain.c | 4 ++--
> src/qemu/qemu_driver.c | 11 -----------
> src/util/virerror.h | 11 +++++++++++
> 4 files changed, 24 insertions(+), 13 deletions(-)
>
...
> diff --git a/src/util/virerror.h b/src/util/virerror.h
> index ad3a946..c1a445e 100644
> --- a/src/util/virerror.h
> +++ b/src/util/virerror.h
> @@ -95,6 +95,17 @@ void virReportSystemErrorFull(int domcode,
> 0, 0, \
> _("%s in %s must not be NULL"), \
> #argname, __FUNCTION__)
> +# define virReportInvalidEmptyStringArg(argname) \
> + virRaiseErrorFull(__FILE__, __FUNCTION__, __LINE__, \
> + VIR_FROM_THIS, \
> + VIR_ERR_INVALID_ARG, \
> + VIR_ERR_ERROR, \
> + __FUNCTION__, \
> + #argname, \
> + NULL, \
> + 0, 0, \
> + _("string %s in %s must not be non empty"), \
This reads strangely... The double negative (not and non)
"string %s in %s must not be empty" or "string %s in %s must be non
empty"
I went with the first option. I goofed when I tried to touch up the
message after stealing it from an existing macro :)
Or more technically
"string %s in %s cannot start with the NUL character" (yes, correct
spelling for NUL - that's the '\0')
Yep, but this option does not sound user-friendly to me.
> + #argname, __FUNCTION__)
> # define virReportInvalidPositiveArg(argname) \
> virRaiseErrorFull(__FILE__, __FUNCTION__, __LINE__, \
> VIR_FROM_THIS, \
>
ACK with some sort of change
Thanks.
Peter
12:15 p.m.
New subject: [libvirt] [PATCH 2/5] qemu: monitor: Fix indentation in qemuMonitorJSONGetOneBlockStatsInfo
---
src/qemu/qemu_monitor_json.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 71e8f4b..11c45a1 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -1725,14 +1725,14 @@ qemuMonitorJSONGetOneBlockStatsInfo(virJSONValuePtr dev,
goto cleanup; \
} \
}
- QEMU_MONITOR_BLOCK_STAT_GET("rd_bytes", bstats->rd_bytes, true);
- QEMU_MONITOR_BLOCK_STAT_GET("wr_bytes", bstats->wr_bytes, true);
- QEMU_MONITOR_BLOCK_STAT_GET("rd_operations", bstats->rd_req, true);
- QEMU_MONITOR_BLOCK_STAT_GET("wr_operations", bstats->wr_req, true);
- QEMU_MONITOR_BLOCK_STAT_GET("rd_total_time_ns", bstats->rd_total_times,
false);
- QEMU_MONITOR_BLOCK_STAT_GET("wr_total_time_ns", bstats->wr_total_times,
false);
- QEMU_MONITOR_BLOCK_STAT_GET("flush_operations", bstats->flush_req,
false);
- QEMU_MONITOR_BLOCK_STAT_GET("flush_total_time_ns",
bstats->flush_total_times, false);
+ QEMU_MONITOR_BLOCK_STAT_GET("rd_bytes", bstats->rd_bytes, true);
+ QEMU_MONITOR_BLOCK_STAT_GET("wr_bytes", bstats->wr_bytes, true);
+ QEMU_MONITOR_BLOCK_STAT_GET("rd_operations", bstats->rd_req, true);
+ QEMU_MONITOR_BLOCK_STAT_GET("wr_operations", bstats->wr_req, true);
+ QEMU_MONITOR_BLOCK_STAT_GET("rd_total_time_ns", bstats->rd_total_times,
false);
+ QEMU_MONITOR_BLOCK_STAT_GET("wr_total_time_ns", bstats->wr_total_times,
false);
+ QEMU_MONITOR_BLOCK_STAT_GET("flush_operations", bstats->flush_req,
false);
+ QEMU_MONITOR_BLOCK_STAT_GET("flush_total_time_ns",
bstats->flush_total_times, false);
#undef QEMU_MONITOR_BLOCK_STAT_GET
/* it's ok to not have this information here. Just skip silently. */
--
2.4.1
12:15 p.m.
New subject: [libvirt] [PATCH 3/5] qemu: monitor: Open-code retrieval of wr_highest_offset
Instead of using qemuMonitorJSONDevGetBlockExtent (which I plan to
remove later) extract the data in place.
Additionally add a flag that will be set when the wr_highest_offset was
extracted correctly so that callers can act according to that.
The test case addition should help make sure that everything works.
---
src/qemu/qemu_monitor.h | 1 +
src/qemu/qemu_monitor_json.c | 10 ++++++++--
tests/qemumonitorjsontest.c | 20 +++++++++++++++-----
tests/qemumonitortest.c | 10 +++++-----
4 files changed, 29 insertions(+), 12 deletions(-)
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 1afc344..b4d6538 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -379,6 +379,7 @@ struct _qemuBlockStats {
unsigned long long capacity;
unsigned long long physical;
unsigned long long wr_highest_offset;
+ bool wr_highest_offset_valid;
};
int qemuMonitorGetAllBlockStatsInfo(qemuMonitorPtr mon,
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 11c45a1..b2ce33f 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -1699,6 +1699,8 @@ qemuMonitorJSONGetOneBlockStatsInfo(virJSONValuePtr dev,
{
qemuBlockStatsPtr bstats = NULL;
virJSONValuePtr stats;
+ virJSONValuePtr parent;
+ virJSONValuePtr parentstats;
int ret = -1;
int nstats = 0;
char *entry_name = qemuDomainStorageAlias(dev_name, depth);
@@ -1735,8 +1737,12 @@ qemuMonitorJSONGetOneBlockStatsInfo(virJSONValuePtr dev,
QEMU_MONITOR_BLOCK_STAT_GET("flush_total_time_ns",
bstats->flush_total_times, false);
#undef QEMU_MONITOR_BLOCK_STAT_GET
- /* it's ok to not have this information here. Just skip silently. */
- qemuMonitorJSONDevGetBlockExtent(dev, &bstats->wr_highest_offset);
+ if ((parent = virJSONValueObjectGetObject(dev, "parent")) &&
+ (parentstats = virJSONValueObjectGetObject(parent, "stats"))) {
+ if (virJSONValueObjectGetNumberUlong(parentstats, "wr_highest_offset",
+ &bstats->wr_highest_offset) == 0)
+ bstats->wr_highest_offset_valid = true;
+ }
if (virHashAddEntry(hash, entry_name, bstats) < 0)
goto cleanup;
diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
index 0623275..6246737 100644
--- a/tests/qemumonitorjsontest.c
+++ b/tests/qemumonitorjsontest.c
@@ -1546,8 +1546,16 @@ testQemuMonitorJSONqemuMonitorJSONGetBlockStatsInfo(const void
*data)
goto cleanup; \
}
+#define CHECK0ULL(var, value) \
+ if (stats->var != value) { \
+ virReportError(VIR_ERR_INTERNAL_ERROR, \
+ "Invalid " #var " value: %llu, expected %llu",
\
+ stats->var, value); \
+ goto cleanup; \
+ }
+
#define CHECK(NAME, RD_REQ, RD_BYTES, RD_TOTAL_TIMES, WR_REQ, WR_BYTES, \
- WR_TOTAL_TIMES, FLUSH_REQ, FLUSH_TOTAL_TIMES) \
+ WR_TOTAL_TIMES, FLUSH_REQ, FLUSH_TOTAL_TIMES, WR_HIGHEST_OFFSET) \
if (!(stats = virHashLookup(blockstats, NAME))) { \
virReportError(VIR_ERR_INTERNAL_ERROR, \
"block stats for device '%s' is missing", NAME);
\
@@ -1560,7 +1568,8 @@ testQemuMonitorJSONqemuMonitorJSONGetBlockStatsInfo(const void
*data)
CHECK0(wr_bytes, WR_BYTES) \
CHECK0(wr_total_times, WR_TOTAL_TIMES) \
CHECK0(flush_req, FLUSH_REQ) \
- CHECK0(flush_total_times, FLUSH_TOTAL_TIMES)
+ CHECK0(flush_total_times, FLUSH_TOTAL_TIMES) \
+ CHECK0ULL(wr_highest_offset, WR_HIGHEST_OFFSET)
if (qemuMonitorGetAllBlockStatsInfo(qemuMonitorTestGetMonitor(test),
&blockstats, false) < 0)
@@ -1572,9 +1581,9 @@ testQemuMonitorJSONqemuMonitorJSONGetBlockStatsInfo(const void
*data)
goto cleanup;
}
- CHECK("virtio-disk0", 1279, 28505088, 640616474, 174, 2845696, 530699221,
0, 0)
- CHECK("virtio-disk1", 85, 348160, 8232156, 0, 0, 0, 0, 0)
- CHECK("ide0-1-0", 16, 49250, 1004952, 0, 0, 0, 0, 0)
+ CHECK("virtio-disk0", 1279, 28505088, 640616474, 174, 2845696, 530699221,
0, 0, 5256018944ULL)
+ CHECK("virtio-disk1", 85, 348160, 8232156, 0, 0, 0, 0, 0, 0ULL)
+ CHECK("ide0-1-0", 16, 49250, 1004952, 0, 0, 0, 0, 0, 0ULL)
if (qemuMonitorJSONGetBlockExtent(qemuMonitorTestGetMonitor(test),
"virtio-disk0",
&extent) < 0)
@@ -1613,6 +1622,7 @@ testQemuMonitorJSONqemuMonitorJSONGetBlockStatsInfo(const void
*data)
#undef CHECK
#undef CHECK0
+#undef CHECK0ULL
cleanup:
qemuMonitorTestFree(test);
diff --git a/tests/qemumonitortest.c b/tests/qemumonitortest.c
index 0125962..9d8d70a 100644
--- a/tests/qemumonitortest.c
+++ b/tests/qemumonitortest.c
@@ -94,11 +94,11 @@ struct blockInfoData {
static const struct blockInfoData testBlockInfoData[] =
{
/* NAME, rd_req, rd_bytes, wr_req, wr_bytes, rd_total_time, wr_total_time, flush_req,
flush_total_time */
- {"vda", {11, 12, 13, 14, 15, 16, 17, 18, 0, 0, 0}},
- {"vdb", {21, 22, 23, 24, 25, 26, 27, 28, 0, 0, 0}},
- {"vdc", {31, 32, 33, -1, 35, 36, 37, 38, 0, 0, 0}},
- {"vdd", {-1, -1, -1, -1, -1, -1, -1, -1, 0, 0, 0}},
- {"vde", {41, 42, 43, 44, 45, 46, 47, 48, 0, 0, 0}}
+ {"vda", {11, 12, 13, 14, 15, 16, 17, 18, 0, 0, 0, false}},
+ {"vdb", {21, 22, 23, 24, 25, 26, 27, 28, 0, 0, 0, false}},
+ {"vdc", {31, 32, 33, -1, 35, 36, 37, 38, 0, 0, 0, false}},
+ {"vdd", {-1, -1, -1, -1, -1, -1, -1, -1, 0, 0, 0, false}},
+ {"vde", {41, 42, 43, 44, 45, 46, 47, 48, 0, 0, 0, false}}
};
static const char testBlockInfoReply[] =
--
2.4.1
11:56 a.m.
New subject: [libvirt] [PATCH 3/5] qemu: monitor: Open-code retrieval of wr_highest_offset
On 06/23/2015 01:15 PM, Peter Krempa wrote:
Instead of using qemuMonitorJSONDevGetBlockExtent (which I plan to
remove later) extract the data in place.
Additionally add a flag that will be set when the wr_highest_offset was
extracted correctly so that callers can act according to that.
The test case addition should help make sure that everything works.
---
src/qemu/qemu_monitor.h | 1 +
src/qemu/qemu_monitor_json.c | 10 ++++++++--
tests/qemumonitorjsontest.c | 20 +++++++++++++++-----
tests/qemumonitortest.c | 10 +++++-----
4 files changed, 29 insertions(+), 12 deletions(-)
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 1afc344..b4d6538 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -379,6 +379,7 @@ struct _qemuBlockStats {
unsigned long long capacity;
unsigned long long physical;
unsigned long long wr_highest_offset;
+ bool wr_highest_offset_valid;
};
Maybe some sort of comment that starting at wr_highest_offset, all
values require both the data and a bool of the same name, but with
_valid ... (see below - perhaps it'll make more sense)
int qemuMonitorGetAllBlockStatsInfo(qemuMonitorPtr mon,
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 11c45a1..b2ce33f 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -1699,6 +1699,8 @@ qemuMonitorJSONGetOneBlockStatsInfo(virJSONValuePtr dev,
{
qemuBlockStatsPtr bstats = NULL;
virJSONValuePtr stats;
+ virJSONValuePtr parent;
+ virJSONValuePtr parentstats;
int ret = -1;
int nstats = 0;
char *entry_name = qemuDomainStorageAlias(dev_name, depth);
@@ -1735,8 +1737,12 @@ qemuMonitorJSONGetOneBlockStatsInfo(virJSONValuePtr dev,
QEMU_MONITOR_BLOCK_STAT_GET("flush_total_time_ns",
bstats->flush_total_times, false);
#undef QEMU_MONITOR_BLOCK_STAT_GET
- /* it's ok to not have this information here. Just skip silently. */
- qemuMonitorJSONDevGetBlockExtent(dev, &bstats->wr_highest_offset);
+ if ((parent = virJSONValueObjectGetObject(dev, "parent")) &&
+ (parentstats = virJSONValueObjectGetObject(parent, "stats"))) {
+ if (virJSONValueObjectGetNumberUlong(parentstats,
"wr_highest_offset",
+ &bstats->wr_highest_offset) == 0)
+ bstats->wr_highest_offset_valid = true;
+ }
if (virHashAddEntry(hash, entry_name, bstats) < 0)
goto cleanup;
diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
index 0623275..6246737 100644
--- a/tests/qemumonitorjsontest.c
+++ b/tests/qemumonitorjsontest.c
@@ -1546,8 +1546,16 @@ testQemuMonitorJSONqemuMonitorJSONGetBlockStatsInfo(const void
*data)
goto cleanup; \
}
+#define CHECK0ULL(var, value) \
+ if (stats->var != value) { \
+ virReportError(VIR_ERR_INTERNAL_ERROR, \
+ "Invalid " #var " value: %llu, expected
%llu", \
+ stats->var, value); \
+ goto cleanup; \
+ }
+
#define CHECK(NAME, RD_REQ, RD_BYTES, RD_TOTAL_TIMES, WR_REQ, WR_BYTES, \
- WR_TOTAL_TIMES, FLUSH_REQ, FLUSH_TOTAL_TIMES) \
+ WR_TOTAL_TIMES, FLUSH_REQ, FLUSH_TOTAL_TIMES, WR_HIGHEST_OFFSET) \
if (!(stats = virHashLookup(blockstats, NAME))) { \
virReportError(VIR_ERR_INTERNAL_ERROR, \
"block stats for device '%s' is missing",
NAME); \
@@ -1560,7 +1568,8 @@ testQemuMonitorJSONqemuMonitorJSONGetBlockStatsInfo(const void
*data)
CHECK0(wr_bytes, WR_BYTES) \
CHECK0(wr_total_times, WR_TOTAL_TIMES) \
CHECK0(flush_req, FLUSH_REQ) \
- CHECK0(flush_total_times, FLUSH_TOTAL_TIMES)
+ CHECK0(flush_total_times, FLUSH_TOTAL_TIMES) \
+ CHECK0ULL(wr_highest_offset, WR_HIGHEST_OFFSET)
if (qemuMonitorGetAllBlockStatsInfo(qemuMonitorTestGetMonitor(test),
&blockstats, false) < 0)
@@ -1572,9 +1581,9 @@ testQemuMonitorJSONqemuMonitorJSONGetBlockStatsInfo(const void
*data)
goto cleanup;
}
- CHECK("virtio-disk0", 1279, 28505088, 640616474, 174, 2845696, 530699221,
0, 0)
- CHECK("virtio-disk1", 85, 348160, 8232156, 0, 0, 0, 0, 0)
- CHECK("ide0-1-0", 16, 49250, 1004952, 0, 0, 0, 0, 0)
+ CHECK("virtio-disk0", 1279, 28505088, 640616474, 174, 2845696, 530699221,
0, 0, 5256018944ULL)
+ CHECK("virtio-disk1", 85, 348160, 8232156, 0, 0, 0, 0, 0, 0ULL)
+ CHECK("ide0-1-0", 16, 49250, 1004952, 0, 0, 0, 0, 0, 0ULL)
This assumes that the wr_highest_offset was found, what about when it's
not found in the data stream?
Seems that would mean either modifying the CHECK macro to add another
parameter (true/false) or some sort of magic in the CHECK0LL macro that
does something like "stats->#var_valid" (I never get the exact syntax
correct without trying a few times) [hence my comment earlier about
modifying the .h file comments to have a field and field_valid bool.
if (qemuMonitorJSONGetBlockExtent(qemuMonitorTestGetMonitor(test),
"virtio-disk0",
&extent) < 0)
@@ -1613,6 +1622,7 @@ testQemuMonitorJSONqemuMonitorJSONGetBlockStatsInfo(const void
*data)
#undef CHECK
#undef CHECK0
+#undef CHECK0ULL
cleanup:
qemuMonitorTestFree(test);
diff --git a/tests/qemumonitortest.c b/tests/qemumonitortest.c
index 0125962..9d8d70a 100644
--- a/tests/qemumonitortest.c
+++ b/tests/qemumonitortest.c
@@ -94,11 +94,11 @@ struct blockInfoData {
static const struct blockInfoData testBlockInfoData[] =
{
/* NAME, rd_req, rd_bytes, wr_req, wr_bytes, rd_total_time, wr_total_time, flush_req,
flush_total_time */
So currently 8 of the 11 params inside the {} are documented the "0, 0,
0" in each wasn't... (eg, capacity, physical, wr_highest_offset) then
adding "false" isn't either.
- {"vda", {11, 12, 13, 14, 15, 16, 17, 18, 0, 0, 0}},
- {"vdb", {21, 22, 23, 24, 25, 26, 27, 28, 0, 0, 0}},
- {"vdc", {31, 32, 33, -1, 35, 36, 37, 38, 0, 0, 0}},
- {"vdd", {-1, -1, -1, -1, -1, -1, -1, -1, 0, 0, 0}},
- {"vde", {41, 42, 43, 44, 45, 46, 47, 48, 0, 0, 0}}
+ {"vda", {11, 12, 13, 14, 15, 16, 17, 18, 0, 0, 0, false}},
+ {"vdb", {21, 22, 23, 24, 25, 26, 27, 28, 0, 0, 0, false}},
+ {"vdc", {31, 32, 33, -1, 35, 36, 37, 38, 0, 0, 0, false}},
+ {"vdd", {-1, -1, -1, -1, -1, -1, -1, -1, 0, 0, 0, false}},
+ {"vde", {41, 42, 43, 44, 45, 46, 47, 48, 0, 0, 0, false}}
Should there perhaps be an entry that tests "true" and places a non zero
value in wr_highest_offset, plus the comparable change to
testBlockInfoReply?
};
static const char testBlockInfoReply[] =
The code seems OK to me - seems the tests need to check for the
wr_highest_offset being true.
ACK with some test adjustments.
John
9:34 a.m.
New subject: [libvirt] [PATCH 3/5] qemu: monitor: Open-code retrieval of wr_highest_offset
On Thu, Jun 25, 2015 at 12:56:02 -0400, John Ferlan wrote:
On 06/23/2015 01:15 PM, Peter Krempa wrote:
> Instead of using qemuMonitorJSONDevGetBlockExtent (which I plan to
> remove later) extract the data in place.
>
> Additionally add a flag that will be set when the wr_highest_offset was
> extracted correctly so that callers can act according to that.
>
> The test case addition should help make sure that everything works.
> ---
> src/qemu/qemu_monitor.h | 1 +
> src/qemu/qemu_monitor_json.c | 10 ++++++++--
> tests/qemumonitorjsontest.c | 20 +++++++++++++++-----
> tests/qemumonitortest.c | 10 +++++-----
> 4 files changed, 29 insertions(+), 12 deletions(-)
>
> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> index 1afc344..b4d6538 100644
> --- a/src/qemu/qemu_monitor.h
> +++ b/src/qemu/qemu_monitor.h
> @@ -379,6 +379,7 @@ struct _qemuBlockStats {
> unsigned long long capacity;
> unsigned long long physical;
> unsigned long long wr_highest_offset;
> + bool wr_highest_offset_valid;
> };
Maybe some sort of comment that starting at wr_highest_offset, all
values require both the data and a bool of the same name, but with
_valid ... (see below - perhaps it'll make more sense)
Fair enough. The main reason for the variable to be present is to allow
differentiating between wr_highest_off being present and 0 and it not
being present.
>
> int qemuMonitorGetAllBlockStatsInfo(qemuMonitorPtr mon,
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 11c45a1..b2ce33f 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -1699,6 +1699,8 @@ qemuMonitorJSONGetOneBlockStatsInfo(virJSONValuePtr dev,
> {
> qemuBlockStatsPtr bstats = NULL;
> virJSONValuePtr stats;
> + virJSONValuePtr parent;
> + virJSONValuePtr parentstats;
> int ret = -1;
> int nstats = 0;
> char *entry_name = qemuDomainStorageAlias(dev_name, depth);
> @@ -1735,8 +1737,12 @@ qemuMonitorJSONGetOneBlockStatsInfo(virJSONValuePtr dev,
> QEMU_MONITOR_BLOCK_STAT_GET("flush_total_time_ns",
bstats->flush_total_times, false);
> #undef QEMU_MONITOR_BLOCK_STAT_GET
>
> - /* it's ok to not have this information here. Just skip silently. */
> - qemuMonitorJSONDevGetBlockExtent(dev, &bstats->wr_highest_offset);
> + if ((parent = virJSONValueObjectGetObject(dev, "parent")) &&
> + (parentstats = virJSONValueObjectGetObject(parent, "stats"))) {
> + if (virJSONValueObjectGetNumberUlong(parentstats,
"wr_highest_offset",
> + &bstats->wr_highest_offset) ==
0)
> + bstats->wr_highest_offset_valid = true;
> + }
>
> if (virHashAddEntry(hash, entry_name, bstats) < 0)
> goto cleanup;
> diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
> index 0623275..6246737 100644
> --- a/tests/qemumonitorjsontest.c
> +++ b/tests/qemumonitorjsontest.c
> @@ -1546,8 +1546,16 @@ testQemuMonitorJSONqemuMonitorJSONGetBlockStatsInfo(const
void *data)
> goto cleanup; \
> }
>
> +#define CHECK0ULL(var, value) \
> + if (stats->var != value) { \
> + virReportError(VIR_ERR_INTERNAL_ERROR, \
> + "Invalid " #var " value: %llu, expected
%llu", \
> + stats->var, value); \
> + goto cleanup; \
> + }
> +
> #define CHECK(NAME, RD_REQ, RD_BYTES, RD_TOTAL_TIMES, WR_REQ, WR_BYTES, \
> - WR_TOTAL_TIMES, FLUSH_REQ, FLUSH_TOTAL_TIMES) \
> + WR_TOTAL_TIMES, FLUSH_REQ, FLUSH_TOTAL_TIMES, WR_HIGHEST_OFFSET) \
> if (!(stats = virHashLookup(blockstats, NAME))) { \
> virReportError(VIR_ERR_INTERNAL_ERROR, \
> "block stats for device '%s' is missing",
NAME); \
> @@ -1560,7 +1568,8 @@ testQemuMonitorJSONqemuMonitorJSONGetBlockStatsInfo(const void
*data)
> CHECK0(wr_bytes, WR_BYTES) \
> CHECK0(wr_total_times, WR_TOTAL_TIMES) \
> CHECK0(flush_req, FLUSH_REQ) \
> - CHECK0(flush_total_times, FLUSH_TOTAL_TIMES)
> + CHECK0(flush_total_times, FLUSH_TOTAL_TIMES) \
> + CHECK0ULL(wr_highest_offset, WR_HIGHEST_OFFSET)
>
> if (qemuMonitorGetAllBlockStatsInfo(qemuMonitorTestGetMonitor(test),
> &blockstats, false) < 0)
> @@ -1572,9 +1581,9 @@ testQemuMonitorJSONqemuMonitorJSONGetBlockStatsInfo(const void
*data)
> goto cleanup;
> }
>
> - CHECK("virtio-disk0", 1279, 28505088, 640616474, 174, 2845696,
530699221, 0, 0)
> - CHECK("virtio-disk1", 85, 348160, 8232156, 0, 0, 0, 0, 0)
> - CHECK("ide0-1-0", 16, 49250, 1004952, 0, 0, 0, 0, 0)
> + CHECK("virtio-disk0", 1279, 28505088, 640616474, 174, 2845696,
530699221, 0, 0, 5256018944ULL)
> + CHECK("virtio-disk1", 85, 348160, 8232156, 0, 0, 0, 0, 0, 0ULL)
> + CHECK("ide0-1-0", 16, 49250, 1004952, 0, 0, 0, 0, 0, 0ULL)
This assumes that the wr_highest_offset was found, what about when it's
not found in the data stream?
Seems that would mean either modifying the CHECK macro to add another
parameter (true/false) or some sort of magic in the CHECK0LL macro that
does something like "stats->#var_valid" (I never get the exact syntax
correct without trying a few times) [hence my comment earlier about
modifying the .h file comments to have a field and field_valid bool.
I'll add a check that will check the _valid field too. The check will be
possible after the last patch since that one expects the
wr_highest_offset field to be present.
>
> if (qemuMonitorJSONGetBlockExtent(qemuMonitorTestGetMonitor(test),
"virtio-disk0",
> &extent) < 0)
> @@ -1613,6 +1622,7 @@ testQemuMonitorJSONqemuMonitorJSONGetBlockStatsInfo(const void
*data)
>
> #undef CHECK
> #undef CHECK0
> +#undef CHECK0ULL
>
> cleanup:
> qemuMonitorTestFree(test);
> diff --git a/tests/qemumonitortest.c b/tests/qemumonitortest.c
> index 0125962..9d8d70a 100644
> --- a/tests/qemumonitortest.c
> +++ b/tests/qemumonitortest.c
> @@ -94,11 +94,11 @@ struct blockInfoData {
> static const struct blockInfoData testBlockInfoData[] =
> {
> /* NAME, rd_req, rd_bytes, wr_req, wr_bytes, rd_total_time, wr_total_time,
flush_req, flush_total_time */
So currently 8 of the 11 params inside the {} are documented the "0, 0,
0" in each wasn't... (eg, capacity, physical, wr_highest_offset) then
adding "false" isn't either.
Hmm, I'll fix that then since doing a separate patch for the test
doesn't make sense.
> - {"vda", {11, 12, 13, 14, 15, 16, 17, 18, 0, 0, 0}},
> - {"vdb", {21, 22, 23, 24, 25, 26, 27, 28, 0, 0, 0}},
> - {"vdc", {31, 32, 33, -1, 35, 36, 37, 38, 0, 0, 0}},
> - {"vdd", {-1, -1, -1, -1, -1, -1, -1, -1, 0, 0, 0}},
> - {"vde", {41, 42, 43, 44, 45, 46, 47, 48, 0, 0, 0}}
> + {"vda", {11, 12, 13, 14, 15, 16, 17, 18, 0, 0, 0, false}},
> + {"vdb", {21, 22, 23, 24, 25, 26, 27, 28, 0, 0, 0, false}},
> + {"vdc", {31, 32, 33, -1, 35, 36, 37, 38, 0, 0, 0, false}},
> + {"vdd", {-1, -1, -1, -1, -1, -1, -1, -1, 0, 0, 0, false}},
> + {"vde", {41, 42, 43, 44, 45, 46, 47, 48, 0, 0, 0, false}}
Should there perhaps be an entry that tests "true" and places a non zero
value in wr_highest_offset, plus the comparable change to
testBlockInfoReply?
Not really. HMP does not populate that variable. As you can see the IMPL
above is done only for QMP.
> };
>
> static const char testBlockInfoReply[] =
>
The code seems OK to me - seems the tests need to check for the
wr_highest_offset being true.
ACK with some test adjustments.
I'll include the patch that will do the requested check above in this
series while pushing under this ACK.
John
Thanks.
Peter
12:15 p.m.
New subject: [libvirt] [PATCH 4/5] qemu: Refactor qemuDomainGetBlockInfo
Change the code so that it queries the monitor when the VM is alive.
---
src/qemu/qemu_driver.c | 90 +++++++++++++++++++++++++++++---------------------
1 file changed, 53 insertions(+), 37 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 004da7e..3da941e 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -11800,10 +11800,12 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
virQEMUDriverPtr driver = dom->conn->privateData;
virDomainObjPtr vm;
int ret = -1;
- virDomainDiskDefPtr disk = NULL;
- virStorageSourcePtr src;
- bool activeFail = false;
+ virDomainDiskDefPtr disk;
virQEMUDriverConfigPtr cfg = NULL;
+ int rc;
+ virHashTablePtr stats = NULL;
+ qemuBlockStats *entry;
+ char *alias;
virCheckFlags(0, -1);
@@ -11815,11 +11817,6 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
if (virDomainGetBlockInfoEnsureACL(dom->conn, vm->def) < 0)
goto cleanup;
- /* Technically, we only need a job if we are going to query the
- * monitor, which is only for active domains that are using
- * non-raw block devices. But it is easier to share code if we
- * always grab a job; furthermore, grabbing the job ensures that
- * hot-plug won't change disk behind our backs. */
if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
goto cleanup;
@@ -11829,53 +11826,72 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
goto endjob;
}
- src = disk->src;
- if (virStorageSourceIsEmpty(src)) {
+ if (virStorageSourceIsEmpty(disk->src)) {
virReportError(VIR_ERR_INVALID_ARG,
_("disk '%s' does not currently have a source
assigned"),
path);
goto endjob;
}
- if ((ret = qemuStorageLimitsRefresh(driver, cfg, vm, src)) < 0)
+ /* for inactive domains we have to peek into the files */
+ if (!virDomainObjIsActive(vm)) {
+ if ((qemuStorageLimitsRefresh(driver, cfg, vm, disk->src)) < 0)
+ goto endjob;
+
+ info->capacity = disk->src->capacity;
+ info->allocation = disk->src->allocation;
+ info->physical = disk->src->physical;
+
+ ret = 0;
+ goto endjob;
+ }
+
+ if (!disk->info.alias ||
+ !(alias = qemuDomainStorageAlias(disk->info.alias, 0))) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("missing disk device alias name for %s"),
disk->dst);
goto endjob;
+ }
- if (!src->allocation) {
- qemuDomainObjPrivatePtr priv = vm->privateData;
+ qemuDomainObjEnterMonitor(driver, vm);
+ rc = qemuMonitorGetAllBlockStatsInfo(qemuDomainGetMonitor(vm),
+ &stats, false);
+ if (rc >= 0)
+ rc = qemuMonitorBlockStatsUpdateCapacity(qemuDomainGetMonitor(vm),
+ stats, false);
- /* If the guest is not running, then success/failure return
- * depends on whether domain is persistent
- */
- if (!virDomainObjIsActive(vm)) {
- activeFail = true;
+ if (qemuDomainObjExitMonitor(driver, vm) < 0 || rc < 0)
+ goto endjob;
+
+ if (!(entry = virHashLookup(stats, alias))) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("failed to gather stats for disk '%s'"),
disk->dst);
+ goto endjob;
+ }
+
+ if (!entry->wr_highest_offset_valid) {
+ if (virStorageSourceGetActualType(disk->src) == VIR_STORAGE_TYPE_BLOCK
&&
+ disk->src->format != VIR_STORAGE_FILE_RAW) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("failed to query the maximum written offset of "
+ "block device '%s'"), disk->dst);
goto endjob;
}
- qemuDomainObjEnterMonitor(driver, vm);
- ret = qemuMonitorGetBlockExtent(priv->mon,
- disk->info.alias,
- &src->allocation);
- if (qemuDomainObjExitMonitor(driver, vm) < 0)
- ret = -1;
+ info->allocation = entry->physical;
+ } else {
+ info->allocation = entry->wr_highest_offset;
}
- if (ret == 0) {
- info->capacity = src->capacity;
- info->allocation = src->allocation;
- info->physical = src->physical;
- }
+ info->capacity = entry->capacity;
+ info->physical = entry->physical;
+
+ ret = 0;
endjob:
qemuDomainObjEndJob(driver, vm);
cleanup:
- /* If we failed to get data from a domain because it's inactive and
- * it's not a persistent domain, then force failure.
- */
- if (activeFail && vm && !vm->persistent) {
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("domain is not running"));
- ret = -1;
- }
+ virHashFree(stats);
virDomainObjEndAPI(&vm);
virObjectUnref(cfg);
return ret;
--
2.4.1
12:26 p.m.
New subject: [libvirt] [PATCH 4/5] qemu: Refactor qemuDomainGetBlockInfo
On 06/23/2015 01:15 PM, Peter Krempa wrote:
Change the code so that it queries the monitor when the VM is alive.
---
src/qemu/qemu_driver.c | 90 +++++++++++++++++++++++++++++---------------------
1 file changed, 53 insertions(+), 37 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 004da7e..3da941e 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -11800,10 +11800,12 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
virQEMUDriverPtr driver = dom->conn->privateData;
virDomainObjPtr vm;
int ret = -1;
- virDomainDiskDefPtr disk = NULL;
- virStorageSourcePtr src;
- bool activeFail = false;
+ virDomainDiskDefPtr disk;
virQEMUDriverConfigPtr cfg = NULL;
+ int rc;
+ virHashTablePtr stats = NULL;
+ qemuBlockStats *entry;
+ char *alias;
virCheckFlags(0, -1);
@@ -11815,11 +11817,6 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
if (virDomainGetBlockInfoEnsureACL(dom->conn, vm->def) < 0)
goto cleanup;
- /* Technically, we only need a job if we are going to query the
- * monitor, which is only for active domains that are using
- * non-raw block devices. But it is easier to share code if we
- * always grab a job; furthermore, grabbing the job ensures that
- * hot-plug won't change disk behind our backs. */
Was the comment wrong? I tend to like comments like this, since it gives
me an understanding why something was done a particular way...
I'm not going to stop a change for removing the comment though...
ACK
John
if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
goto cleanup;
@@ -11829,53 +11826,72 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
goto endjob;
}
- src = disk->src;
- if (virStorageSourceIsEmpty(src)) {
+ if (virStorageSourceIsEmpty(disk->src)) {
virReportError(VIR_ERR_INVALID_ARG,
_("disk '%s' does not currently have a source
assigned"),
path);
goto endjob;
}
- if ((ret = qemuStorageLimitsRefresh(driver, cfg, vm, src)) < 0)
+ /* for inactive domains we have to peek into the files */
+ if (!virDomainObjIsActive(vm)) {
+ if ((qemuStorageLimitsRefresh(driver, cfg, vm, disk->src)) < 0)
+ goto endjob;
+
+ info->capacity = disk->src->capacity;
+ info->allocation = disk->src->allocation;
+ info->physical = disk->src->physical;
+
+ ret = 0;
+ goto endjob;
+ }
+
+ if (!disk->info.alias ||
+ !(alias = qemuDomainStorageAlias(disk->info.alias, 0))) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("missing disk device alias name for %s"),
disk->dst);
goto endjob;
+ }
- if (!src->allocation) {
- qemuDomainObjPrivatePtr priv = vm->privateData;
+ qemuDomainObjEnterMonitor(driver, vm);
+ rc = qemuMonitorGetAllBlockStatsInfo(qemuDomainGetMonitor(vm),
+ &stats, false);
+ if (rc >= 0)
+ rc = qemuMonitorBlockStatsUpdateCapacity(qemuDomainGetMonitor(vm),
+ stats, false);
- /* If the guest is not running, then success/failure return
- * depends on whether domain is persistent
- */
- if (!virDomainObjIsActive(vm)) {
- activeFail = true;
+ if (qemuDomainObjExitMonitor(driver, vm) < 0 || rc < 0)
+ goto endjob;
+
+ if (!(entry = virHashLookup(stats, alias))) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("failed to gather stats for disk '%s'"),
disk->dst);
+ goto endjob;
+ }
+
+ if (!entry->wr_highest_offset_valid) {
+ if (virStorageSourceGetActualType(disk->src) == VIR_STORAGE_TYPE_BLOCK
&&
+ disk->src->format != VIR_STORAGE_FILE_RAW) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("failed to query the maximum written offset of "
+ "block device '%s'"), disk->dst);
goto endjob;
}
- qemuDomainObjEnterMonitor(driver, vm);
- ret = qemuMonitorGetBlockExtent(priv->mon,
- disk->info.alias,
- &src->allocation);
- if (qemuDomainObjExitMonitor(driver, vm) < 0)
- ret = -1;
+ info->allocation = entry->physical;
+ } else {
+ info->allocation = entry->wr_highest_offset;
}
- if (ret == 0) {
- info->capacity = src->capacity;
- info->allocation = src->allocation;
- info->physical = src->physical;
- }
+ info->capacity = entry->capacity;
+ info->physical = entry->physical;
+
+ ret = 0;
endjob:
qemuDomainObjEndJob(driver, vm);
cleanup:
- /* If we failed to get data from a domain because it's inactive and
- * it's not a persistent domain, then force failure.
- */
- if (activeFail && vm && !vm->persistent) {
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("domain is not running"));
- ret = -1;
- }
+ virHashFree(stats);
virDomainObjEndAPI(&vm);
virObjectUnref(cfg);
return ret;
1:01 p.m.
New subject: [libvirt] [PATCH 4/5] qemu: Refactor qemuDomainGetBlockInfo
On Thu, Jun 25, 2015 at 13:26:35 -0400, John Ferlan wrote:
On 06/23/2015 01:15 PM, Peter Krempa wrote:
> Change the code so that it queries the monitor when the VM is alive.
> ---
> src/qemu/qemu_driver.c | 90 +++++++++++++++++++++++++++++---------------------
> 1 file changed, 53 insertions(+), 37 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 004da7e..3da941e 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -11800,10 +11800,12 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
> virQEMUDriverPtr driver = dom->conn->privateData;
> virDomainObjPtr vm;
> int ret = -1;
> - virDomainDiskDefPtr disk = NULL;
> - virStorageSourcePtr src;
> - bool activeFail = false;
> + virDomainDiskDefPtr disk;
> virQEMUDriverConfigPtr cfg = NULL;
> + int rc;
> + virHashTablePtr stats = NULL;
> + qemuBlockStats *entry;
> + char *alias;
>
> virCheckFlags(0, -1);
>
> @@ -11815,11 +11817,6 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
> if (virDomainGetBlockInfoEnsureACL(dom->conn, vm->def) < 0)
> goto cleanup;
>
> - /* Technically, we only need a job if we are going to query the
> - * monitor, which is only for active domains that are using
> - * non-raw block devices. But it is easier to share code if we
> - * always grab a job; furthermore, grabbing the job ensures that
> - * hot-plug won't change disk behind our backs. */
Was the comment wrong? I tend to like comments like this, since it gives
me an understanding why something was done a particular way...
qemuStorageLimitsRefresh modifies few fields of the disk->src structure.
They are only integer modifications of capacity and they are guarded by
the vm lock but that still should be covered by a job semantically.
There are only very few exceptions to that rule.
Additionally, grabbing the job helps to prevent the situation where we'd
check that the VM is alive (where we do need a job to query the
monitor), then we'd enter the job, but the vm would die at that precise
moment and the rollback path from that situation would be convoluted.
Peter
12:15 p.m.
New subject: [libvirt] [PATCH 5/5] qemu: monitor: Remove qemuMonitorGetBlockExtent
Now that qemuMonitorGetAllBlockStatsInfo collects also wr_highest_offset
the whole function can be killed.
---
src/qemu/qemu_monitor.c | 16 -----
src/qemu/qemu_monitor.h | 3 -
src/qemu/qemu_monitor_json.c | 138 -------------------------------------------
src/qemu/qemu_monitor_json.h | 3 -
src/qemu/qemu_monitor_text.c | 10 ----
src/qemu/qemu_monitor_text.h | 3 -
tests/qemumonitorjsontest.c | 34 -----------
7 files changed, 207 deletions(-)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 904d682..93fcc7f 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -1829,22 +1829,6 @@ qemuMonitorBlockStatsUpdateCapacity(qemuMonitorPtr mon,
int
-qemuMonitorGetBlockExtent(qemuMonitorPtr mon,
- const char *dev_name,
- unsigned long long *extent)
-{
- VIR_DEBUG("dev_name=%s", dev_name);
-
- QEMU_CHECK_MONITOR(mon);
-
- if (mon->json)
- return qemuMonitorJSONGetBlockExtent(mon, dev_name, extent);
- else
- return qemuMonitorTextGetBlockExtent(mon, dev_name, extent);
-}
-
-
-int
qemuMonitorBlockResize(qemuMonitorPtr mon,
const char *device,
unsigned long long size)
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index b4d6538..483496c 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -392,9 +392,6 @@ int qemuMonitorBlockStatsUpdateCapacity(qemuMonitorPtr mon,
bool backingChain)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
-int qemuMonitorGetBlockExtent(qemuMonitorPtr mon,
- const char *dev_name,
- unsigned long long *extent);
int qemuMonitorBlockResize(qemuMonitorPtr mon,
const char *dev_name,
unsigned long long size);
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index b2ce33f..d496a32 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -1660,36 +1660,6 @@ int qemuMonitorJSONGetBlockInfo(qemuMonitorPtr mon,
}
-typedef enum {
- QEMU_MONITOR_BLOCK_EXTENT_ERROR_OK,
- QEMU_MONITOR_BLOCK_EXTENT_ERROR_NOPARENT,
- QEMU_MONITOR_BLOCK_EXTENT_ERROR_NOSTATS,
- QEMU_MONITOR_BLOCK_EXTENT_ERROR_NOOFFSET,
-} qemuMonitorBlockExtentError;
-
-
-static int
-qemuMonitorJSONDevGetBlockExtent(virJSONValuePtr dev,
- unsigned long long *extent)
-{
- virJSONValuePtr stats;
- virJSONValuePtr parent;
-
- if ((parent = virJSONValueObjectGetObject(dev, "parent")) == NULL)
- return QEMU_MONITOR_BLOCK_EXTENT_ERROR_NOPARENT;
-
- if ((stats = virJSONValueObjectGetObject(parent, "stats")) == NULL)
- return QEMU_MONITOR_BLOCK_EXTENT_ERROR_NOSTATS;
-
- if (virJSONValueObjectGetNumberUlong(stats, "wr_highest_offset",
- extent) < 0) {
- return QEMU_MONITOR_BLOCK_EXTENT_ERROR_NOOFFSET;
- }
-
- return QEMU_MONITOR_BLOCK_EXTENT_ERROR_OK;
-}
-
-
static int
qemuMonitorJSONGetOneBlockStatsInfo(virJSONValuePtr dev,
const char *dev_name,
@@ -1943,114 +1913,6 @@ qemuMonitorJSONBlockStatsUpdateCapacity(qemuMonitorPtr mon,
}
-static int
-qemuMonitorJSONReportBlockExtentError(qemuMonitorBlockExtentError error)
-{
- switch (error) {
- case QEMU_MONITOR_BLOCK_EXTENT_ERROR_NOPARENT:
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("blockstats parent entry was not in "
- "expected format"));
- break;
-
- case QEMU_MONITOR_BLOCK_EXTENT_ERROR_NOSTATS:
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("blockstats stats entry was not in "
- "expected format"));
- break;
-
- case QEMU_MONITOR_BLOCK_EXTENT_ERROR_NOOFFSET:
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("cannot read %s statistic"),
- "wr_highest_offset");
- break;
-
- case QEMU_MONITOR_BLOCK_EXTENT_ERROR_OK:
- return 0;
- }
-
- return -1;
-}
-
-
-int qemuMonitorJSONGetBlockExtent(qemuMonitorPtr mon,
- const char *dev_name,
- unsigned long long *extent)
-{
- int ret = -1;
- size_t i;
- bool found = false;
- virJSONValuePtr cmd = qemuMonitorJSONMakeCommand("query-blockstats",
- NULL);
- virJSONValuePtr reply = NULL;
- virJSONValuePtr devices;
-
- *extent = 0;
-
- if (!cmd)
- return -1;
-
- ret = qemuMonitorJSONCommand(mon, cmd, &reply);
-
- if (ret == 0)
- ret = qemuMonitorJSONCheckError(cmd, reply);
- if (ret < 0)
- goto cleanup;
- ret = -1;
-
- if (!(devices = virJSONValueObjectGetArray(reply, "return"))) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("blockstats reply was missing device list"));
- goto cleanup;
- }
-
- for (i = 0; i < virJSONValueArraySize(devices); i++) {
- virJSONValuePtr dev = virJSONValueArrayGet(devices, i);
- const char *thisdev;
- int err;
- if (!dev || dev->type != VIR_JSON_TYPE_OBJECT) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("blockstats device entry was not in expected
format"));
- goto cleanup;
- }
-
- if ((thisdev = virJSONValueObjectGetString(dev, "device")) == NULL) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("blockstats device entry was not in expected
format"));
- goto cleanup;
- }
-
- /* New QEMU has separate names for host & guest side of the disk
- * and libvirt gives the host side a 'drive-' prefix. The passed
- * in dev_name is the guest side though
- */
- if (STRPREFIX(thisdev, QEMU_DRIVE_HOST_PREFIX))
- thisdev += strlen(QEMU_DRIVE_HOST_PREFIX);
-
- if (STRNEQ(thisdev, dev_name))
- continue;
-
- found = true;
- if ((err = qemuMonitorJSONDevGetBlockExtent(dev, extent)) !=
- QEMU_MONITOR_BLOCK_EXTENT_ERROR_OK) {
- qemuMonitorJSONReportBlockExtentError(err);
- goto cleanup;
- }
- }
-
- if (!found) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("cannot find statistics for device '%s'"),
dev_name);
- goto cleanup;
- }
- ret = 0;
-
- cleanup:
- virJSONValueFree(cmd);
- virJSONValueFree(reply);
- return ret;
-}
-
/* Return 0 on success, -1 on failure, or -2 if not supported. Size
* is in bytes. */
int qemuMonitorJSONBlockResize(qemuMonitorPtr mon,
diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
index c0ee4ce..b76d85b 100644
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -77,9 +77,6 @@ int qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitorPtr mon,
int qemuMonitorJSONBlockStatsUpdateCapacity(qemuMonitorPtr mon,
virHashTablePtr stats,
bool backingChain);
-int qemuMonitorJSONGetBlockExtent(qemuMonitorPtr mon,
- const char *dev_name,
- unsigned long long *extent);
int qemuMonitorJSONBlockResize(qemuMonitorPtr mon,
const char *devce,
unsigned long long size);
diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c
index 1b6bc02..2e77534 100644
--- a/src/qemu/qemu_monitor_text.c
+++ b/src/qemu/qemu_monitor_text.c
@@ -968,16 +968,6 @@ qemuMonitorTextGetAllBlockStatsInfo(qemuMonitorPtr mon,
return ret;
}
-
-int qemuMonitorTextGetBlockExtent(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
- const char *dev_name ATTRIBUTE_UNUSED,
- unsigned long long *extent ATTRIBUTE_UNUSED)
-{
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("unable to query block extent with this QEMU"));
- return -1;
-}
-
/* Return 0 on success, -1 on failure, or -2 if not supported. Size
* is in bytes. */
int qemuMonitorTextBlockResize(qemuMonitorPtr mon,
diff --git a/src/qemu/qemu_monitor_text.h b/src/qemu/qemu_monitor_text.h
index 6f9a215..3fa603b 100644
--- a/src/qemu/qemu_monitor_text.h
+++ b/src/qemu/qemu_monitor_text.h
@@ -63,9 +63,6 @@ int qemuMonitorTextGetBlockInfo(qemuMonitorPtr mon,
int qemuMonitorTextGetAllBlockStatsInfo(qemuMonitorPtr mon,
virHashTablePtr hash);
-int qemuMonitorTextGetBlockExtent(qemuMonitorPtr mon,
- const char *dev_name,
- unsigned long long *extent);
int qemuMonitorTextBlockResize(qemuMonitorPtr mon,
const char *device,
unsigned long long size);
diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
index 6246737..6df91b3 100644
--- a/tests/qemumonitorjsontest.c
+++ b/tests/qemumonitorjsontest.c
@@ -1438,7 +1438,6 @@ testQemuMonitorJSONqemuMonitorJSONGetBlockStatsInfo(const void
*data)
virHashTablePtr blockstats = NULL;
qemuBlockStatsPtr stats;
int ret = -1;
- unsigned long long extent;
const char *reply =
"{"
@@ -1585,39 +1584,6 @@ testQemuMonitorJSONqemuMonitorJSONGetBlockStatsInfo(const void
*data)
CHECK("virtio-disk1", 85, 348160, 8232156, 0, 0, 0, 0, 0, 0ULL)
CHECK("ide0-1-0", 16, 49250, 1004952, 0, 0, 0, 0, 0, 0ULL)
- if (qemuMonitorJSONGetBlockExtent(qemuMonitorTestGetMonitor(test),
"virtio-disk0",
- &extent) < 0)
- goto cleanup;
-
- if (extent != 5256018944ULL) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- "Invalid extent: %llu, expected 5256018944",
- extent);
- goto cleanup;
- }
-
- if (qemuMonitorJSONGetBlockExtent(qemuMonitorTestGetMonitor(test),
"virtio-disk1",
- &extent) < 0)
- goto cleanup;
-
- if (extent != 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- "Invalid extent: %llu, expected 0",
- extent);
- goto cleanup;
- }
-
- if (qemuMonitorJSONGetBlockExtent(qemuMonitorTestGetMonitor(test),
"ide0-1-0",
- &extent) < 0)
- goto cleanup;
-
- if (extent != 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- "Invalid extent: %llu, expected 0",
- extent);
- goto cleanup;
- }
-
ret = 0;
#undef CHECK
--
2.4.1
12:28 p.m.
New subject: [libvirt] [PATCH 5/5] qemu: monitor: Remove qemuMonitorGetBlockExtent
On 06/23/2015 01:15 PM, Peter Krempa wrote:
Now that qemuMonitorGetAllBlockStatsInfo collects also
wr_highest_offset
the whole function can be killed.
---
src/qemu/qemu_monitor.c | 16 -----
src/qemu/qemu_monitor.h | 3 -
src/qemu/qemu_monitor_json.c | 138 -------------------------------------------
src/qemu/qemu_monitor_json.h | 3 -
src/qemu/qemu_monitor_text.c | 10 ----
src/qemu/qemu_monitor_text.h | 3 -
tests/qemumonitorjsontest.c | 34 -----------
7 files changed, 207 deletions(-)
ACK
John
3437
days inactive
3440
days old
13 comments
2 participants
participants (2)
-
John Ferlan -
Peter Krempa