[libvirt] [PATCH v3 0/4] Combine various query-block json call paths
I'm not unreasonable ;-) - making the suggested alterations from the review and NACK of v2/v1 which combined the for loops as well. v2: http://www.redhat.com/archives/libvir-list/2016-September/msg01483.html Changes - new patch 1, 3, 4 Alteration to patch 1 of previous series to use the new Steal API John Ferlan (4): util: Introduce virJSONValueObjectStealArray qemu: Create common code for JSON "query-block" call qemu: Create helper qemuMonitorJSONGetBlockDev qemu: Create helper qemuMonitorJSONGetBlockDevDevice src/libvirt_private.syms | 1 + src/qemu/qemu_monitor_json.c | 157 ++++++++++++++++++++++--------------------- src/util/virjson.c | 43 ++++++++++++ src/util/virjson.h | 2 + 4 files changed, 125 insertions(+), 78 deletions(-) -- 2.7.4
Provide the Steal API for any code paths that will desire to grab the entire array and then free it afterwards rather than relying to freeing the whole chain from the reply. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virjson.c | 43 +++++++++++++++++++++++++++++++++++++++++++ src/util/virjson.h | 2 ++ 3 files changed, 46 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index eae817d..c4af57d 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1821,6 +1821,7 @@ virJSONValueObjectHasKey; virJSONValueObjectIsNull; virJSONValueObjectKeysNumber; virJSONValueObjectRemoveKey; +virJSONValueObjectStealArray; virJSONValueToString; diff --git a/src/util/virjson.c b/src/util/virjson.c index b6d9a34..e705f53 100644 --- a/src/util/virjson.c +++ b/src/util/virjson.c @@ -770,6 +770,27 @@ virJSONValueObjectGet(virJSONValuePtr object, } +static virJSONValuePtr +virJSONValueObjectSteal(virJSONValuePtr object, + const char *key) +{ + size_t i; + virJSONValuePtr obj = NULL; + + if (object->type != VIR_JSON_TYPE_OBJECT) + return NULL; + + for (i = 0; i < object->data.object.npairs; i++) { + if (STREQ(object->data.object.pairs[i].key, key)) { + VIR_STEAL_PTR(obj, object->data.object.pairs[i].value); + break; + } + } + + return obj; +} + + /* Return the value associated with KEY within OBJECT, but return NULL * if the key is missing or if value is not the correct TYPE. */ virJSONValuePtr @@ -785,6 +806,21 @@ virJSONValueObjectGetByType(virJSONValuePtr object, } +/* Steal the value associated with KEY within OBJECT, but return NULL + * if the key is missing or if value is not the correct TYPE. */ +static virJSONValuePtr +virJSONValueObjectStealByType(virJSONValuePtr object, + const char *key, + virJSONType type) +{ + virJSONValuePtr value = virJSONValueObjectSteal(object, key); + + if (value && value->type == type) + return value; + return NULL; +} + + int virJSONValueObjectKeysNumber(virJSONValuePtr object) { @@ -1194,6 +1230,13 @@ virJSONValueObjectGetArray(virJSONValuePtr object, const char *key) } +virJSONValuePtr +virJSONValueObjectStealArray(virJSONValuePtr object, const char *key) +{ + return virJSONValueObjectStealByType(object, key, VIR_JSON_TYPE_ARRAY); +} + + int virJSONValueObjectIsNull(virJSONValuePtr object, const char *key) diff --git a/src/util/virjson.h b/src/util/virjson.h index 64cae88..8b62d65 100644 --- a/src/util/virjson.h +++ b/src/util/virjson.h @@ -136,6 +136,8 @@ virJSONValuePtr virJSONValueObjectGetObject(virJSONValuePtr object, const char *key); virJSONValuePtr virJSONValueObjectGetArray(virJSONValuePtr object, const char *key); +virJSONValuePtr virJSONValueObjectStealArray(virJSONValuePtr object, + const char *key); const char *virJSONValueObjectGetString(virJSONValuePtr object, const char *key); int virJSONValueObjectGetNumberInt(virJSONValuePtr object, const char *key, int *value); -- 2.7.4
On Tue, Oct 04, 2016 at 11:28:55 -0400, John Ferlan wrote:
Provide the Steal API for any code paths that will desire to grab the entire array and then free it afterwards rather than relying to freeing the whole chain from the reply.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virjson.c | 43 +++++++++++++++++++++++++++++++++++++++++++ src/util/virjson.h | 2 ++ 3 files changed, 46 insertions(+)
[...]
diff --git a/src/util/virjson.c b/src/util/virjson.c index b6d9a34..e705f53 100644 --- a/src/util/virjson.c +++ b/src/util/virjson.c @@ -770,6 +770,27 @@ virJSONValueObjectGet(virJSONValuePtr object, }
+static virJSONValuePtr +virJSONValueObjectSteal(virJSONValuePtr object, + const char *key) +{ + size_t i; + virJSONValuePtr obj = NULL; + + if (object->type != VIR_JSON_TYPE_OBJECT) + return NULL; + + for (i = 0; i < object->data.object.npairs; i++) { + if (STREQ(object->data.object.pairs[i].key, key)) { + VIR_STEAL_PTR(obj, object->data.object.pairs[i].value);
You steal the data but don't delete the key from the object which makes the object invalid.
+ break; + } + } + + return obj; +}
Peter
On 10/05/2016 03:48 AM, Peter Krempa wrote:
On Tue, Oct 04, 2016 at 11:28:55 -0400, John Ferlan wrote:
Provide the Steal API for any code paths that will desire to grab the entire array and then free it afterwards rather than relying to freeing the whole chain from the reply.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virjson.c | 43 +++++++++++++++++++++++++++++++++++++++++++ src/util/virjson.h | 2 ++ 3 files changed, 46 insertions(+)
[...]
diff --git a/src/util/virjson.c b/src/util/virjson.c index b6d9a34..e705f53 100644 --- a/src/util/virjson.c +++ b/src/util/virjson.c @@ -770,6 +770,27 @@ virJSONValueObjectGet(virJSONValuePtr object, }
+static virJSONValuePtr +virJSONValueObjectSteal(virJSONValuePtr object, + const char *key) +{ + size_t i; + virJSONValuePtr obj = NULL; + + if (object->type != VIR_JSON_TYPE_OBJECT) + return NULL; + + for (i = 0; i < object->data.object.npairs; i++) { + if (STREQ(object->data.object.pairs[i].key, key)) { + VIR_STEAL_PTR(obj, object->data.object.pairs[i].value);
You steal the data but don't delete the key from the object which makes the object invalid.
D'oh - squash this in? diff --git a/src/util/virjson.c b/src/util/virjson.c index e705f53..1d8e6d5 100644 --- a/src/util/virjson.c +++ b/src/util/virjson.c @@ -783,6 +783,9 @@ virJSONValueObjectSteal(virJSONValuePtr object, for (i = 0; i < object->data.object.npairs; i++) { if (STREQ(object->data.object.pairs[i].key, key)) { VIR_STEAL_PTR(obj, object->data.object.pairs[i].value); + VIR_FREE(object->data.object.pairs[i].key); + VIR_DELETE_ELEMENT(object->data.object.pairs, i, + object->data.object.npairs); break; } } John
+ break; + } + } + + return obj; +}
Peter
On Wed, Oct 05, 2016 at 09:47:15 -0400, John Ferlan wrote:
On 10/05/2016 03:48 AM, Peter Krempa wrote:
On Tue, Oct 04, 2016 at 11:28:55 -0400, John Ferlan wrote:
Provide the Steal API for any code paths that will desire to grab the entire array and then free it afterwards rather than relying to freeing the whole chain from the reply.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virjson.c | 43 +++++++++++++++++++++++++++++++++++++++++++ src/util/virjson.h | 2 ++ 3 files changed, 46 insertions(+)
[...]
diff --git a/src/util/virjson.c b/src/util/virjson.c index b6d9a34..e705f53 100644 --- a/src/util/virjson.c +++ b/src/util/virjson.c @@ -770,6 +770,27 @@ virJSONValueObjectGet(virJSONValuePtr object, }
+static virJSONValuePtr +virJSONValueObjectSteal(virJSONValuePtr object, + const char *key) +{ + size_t i; + virJSONValuePtr obj = NULL; + + if (object->type != VIR_JSON_TYPE_OBJECT) + return NULL; + + for (i = 0; i < object->data.object.npairs; i++) { + if (STREQ(object->data.object.pairs[i].key, key)) { + VIR_STEAL_PTR(obj, object->data.object.pairs[i].value);
You steal the data but don't delete the key from the object which makes the object invalid.
D'oh - squash this in?
diff --git a/src/util/virjson.c b/src/util/virjson.c index e705f53..1d8e6d5 100644 --- a/src/util/virjson.c +++ b/src/util/virjson.c @@ -783,6 +783,9 @@ virJSONValueObjectSteal(virJSONValuePtr object, for (i = 0; i < object->data.object.npairs; i++) { if (STREQ(object->data.object.pairs[i].key, key)) { VIR_STEAL_PTR(obj, object->data.object.pairs[i].value); + VIR_FREE(object->data.object.pairs[i].key); + VIR_DELETE_ELEMENT(object->data.object.pairs, i, + object->data.object.npairs); break; } }
yep, ACK
Reduce some cut-n-paste code by creating common helper. Make use of the recently added virJSONValueObjectStealArray to grab the devices list as part of the common code (we we can Free the reply) and return devices for each of the callers to continue to parse. NB: This also adds error checking to qemuMonitorJSONDiskNameLookup Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_monitor_json.c | 91 ++++++++++++++++++++------------------------ 1 file changed, 41 insertions(+), 50 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index e1494df..a5e7ddc 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -1772,32 +1772,52 @@ qemuMonitorJSONSetMemoryStatsPeriod(qemuMonitorPtr mon, } -int qemuMonitorJSONGetBlockInfo(qemuMonitorPtr mon, - virHashTablePtr table) +/* qemuMonitorJSONQueryBlock: + * @mon: Monitor pointer + * + * This helper will attempt to make a "query-block" call and check for + * errors before returning with the reply. + * + * Returns: NULL on error, reply on success + */ +static virJSONValuePtr +qemuMonitorJSONQueryBlock(qemuMonitorPtr mon) { - int ret = -1; - size_t i; - - virJSONValuePtr cmd = qemuMonitorJSONMakeCommand("query-block", - NULL); + virJSONValuePtr cmd; virJSONValuePtr reply = NULL; - virJSONValuePtr devices; + virJSONValuePtr devices = NULL; - if (!cmd) - return -1; - - if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) - goto cleanup; + if (!(cmd = qemuMonitorJSONMakeCommand("query-block", NULL))) + return NULL; - if (qemuMonitorJSONCheckError(cmd, reply) < 0) + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0 || + qemuMonitorJSONCheckError(cmd, reply) < 0) goto cleanup; - if (!(devices = virJSONValueObjectGetArray(reply, "return"))) { + if (!(devices = virJSONValueObjectStealArray(reply, "return"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("block info reply was missing device list")); + _("query-block reply was missing device list")); goto cleanup; } + cleanup: + virJSONValueFree(cmd); + virJSONValueFree(reply); + return devices; +} + + +int qemuMonitorJSONGetBlockInfo(qemuMonitorPtr mon, + virHashTablePtr table) +{ + int ret = -1; + size_t i; + + virJSONValuePtr devices; + + if (!(devices = qemuMonitorJSONQueryBlock(mon))) + return -1; + for (i = 0; i < virJSONValueArraySize(devices); i++) { virJSONValuePtr dev = virJSONValueArrayGet(devices, i); struct qemuDomainDiskInfo *info; @@ -1858,8 +1878,7 @@ int qemuMonitorJSONGetBlockInfo(qemuMonitorPtr mon, ret = 0; cleanup: - virJSONValueFree(cmd); - virJSONValueFree(reply); + virJSONValueFree(devices); return ret; } @@ -2056,27 +2075,12 @@ qemuMonitorJSONBlockStatsUpdateCapacity(qemuMonitorPtr mon, bool backingChain) { int ret = -1; - int rc; size_t i; - virJSONValuePtr cmd; - virJSONValuePtr reply = NULL; virJSONValuePtr devices; - if (!(cmd = qemuMonitorJSONMakeCommand("query-block", NULL))) + if (!(devices = qemuMonitorJSONQueryBlock(mon))) return -1; - if ((rc = qemuMonitorJSONCommand(mon, cmd, &reply)) < 0) - goto cleanup; - - if (qemuMonitorJSONCheckError(cmd, reply) < 0) - goto cleanup; - - if (!(devices = virJSONValueObjectGetArray(reply, "return"))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("query-block reply was missing device list")); - goto cleanup; - } - for (i = 0; i < virJSONValueArraySize(devices); i++) { virJSONValuePtr dev = virJSONValueArrayGet(devices, i); virJSONValuePtr inserted; @@ -2111,8 +2115,7 @@ qemuMonitorJSONBlockStatsUpdateCapacity(qemuMonitorPtr mon, ret = 0; cleanup: - virJSONValueFree(cmd); - virJSONValueFree(reply); + virJSONValueFree(devices); return ret; } @@ -3987,22 +3990,11 @@ qemuMonitorJSONDiskNameLookup(qemuMonitorPtr mon, virStorageSourcePtr target) { char *ret = NULL; - virJSONValuePtr cmd = NULL; - virJSONValuePtr reply = NULL; virJSONValuePtr devices; size_t i; - cmd = qemuMonitorJSONMakeCommand("query-block", NULL); - if (!cmd) + if (!(devices = qemuMonitorJSONQueryBlock(mon))) return NULL; - if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) - goto cleanup; - - if (!(devices = virJSONValueObjectGetArray(reply, "return"))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("block info reply was missing device list")); - goto cleanup; - } for (i = 0; i < virJSONValueArraySize(devices); i++) { virJSONValuePtr dev = virJSONValueArrayGet(devices, i); @@ -4038,8 +4030,7 @@ qemuMonitorJSONDiskNameLookup(qemuMonitorPtr mon, device); cleanup: - virJSONValueFree(cmd); - virJSONValueFree(reply); + virJSONValueFree(devices); return ret; } -- 2.7.4
On Tue, Oct 04, 2016 at 11:28:56 -0400, John Ferlan wrote:
Reduce some cut-n-paste code by creating common helper. Make use of the recently added virJSONValueObjectStealArray to grab the devices list as part of the common code (we we can Free the reply) and return devices for each of the callers to continue to parse.
NB: This also adds error checking to qemuMonitorJSONDiskNameLookup
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_monitor_json.c | 91 ++++++++++++++++++++------------------------ 1 file changed, 41 insertions(+), 50 deletions(-)
ACK
This will grab the 'dev' from devices and do the common validation checks. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_monitor_json.c | 37 +++++++++++++++++++++---------------- 1 file changed, 21 insertions(+), 16 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index a5e7ddc..8cb3891 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -1807,6 +1807,21 @@ qemuMonitorJSONQueryBlock(qemuMonitorPtr mon) } +static virJSONValuePtr +qemuMonitorJSONGetBlockDev(virJSONValuePtr devices, + size_t idx) +{ + virJSONValuePtr dev = virJSONValueArrayGet(devices, idx); + + if (!dev || dev->type != VIR_JSON_TYPE_OBJECT) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("query-block device entry was not in expected format")); + return NULL; + } + return dev; +} + + int qemuMonitorJSONGetBlockInfo(qemuMonitorPtr mon, virHashTablePtr table) { @@ -1819,16 +1834,13 @@ int qemuMonitorJSONGetBlockInfo(qemuMonitorPtr mon, return -1; for (i = 0; i < virJSONValueArraySize(devices); i++) { - virJSONValuePtr dev = virJSONValueArrayGet(devices, i); + virJSONValuePtr dev; struct qemuDomainDiskInfo *info; const char *thisdev; const char *status; - if (!dev || dev->type != VIR_JSON_TYPE_OBJECT) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("block info device entry was not in expected format")); + if (!(dev = qemuMonitorJSONGetBlockDev(devices, i))) goto cleanup; - } if ((thisdev = virJSONValueObjectGetString(dev, "device")) == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -2082,17 +2094,13 @@ qemuMonitorJSONBlockStatsUpdateCapacity(qemuMonitorPtr mon, return -1; for (i = 0; i < virJSONValueArraySize(devices); i++) { - virJSONValuePtr dev = virJSONValueArrayGet(devices, i); + virJSONValuePtr dev; virJSONValuePtr inserted; virJSONValuePtr image; const char *dev_name; - if (!dev || dev->type != VIR_JSON_TYPE_OBJECT) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("query-block device entry was not " - "in expected format")); + if (!(dev = qemuMonitorJSONGetBlockDev(devices, i))) goto cleanup; - } if (!(dev_name = virJSONValueObjectGetString(dev, "device"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -3997,16 +4005,13 @@ qemuMonitorJSONDiskNameLookup(qemuMonitorPtr mon, return NULL; for (i = 0; i < virJSONValueArraySize(devices); i++) { - virJSONValuePtr dev = virJSONValueArrayGet(devices, i); + virJSONValuePtr dev; virJSONValuePtr inserted; virJSONValuePtr image; const char *thisdev; - if (!dev || dev->type != VIR_JSON_TYPE_OBJECT) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("block info device entry was not in expected format")); + if (!(dev = qemuMonitorJSONGetBlockDev(devices, i))) goto cleanup; - } if (!(thisdev = virJSONValueObjectGetString(dev, "device"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", -- 2.7.4
On Tue, Oct 04, 2016 at 11:28:57 -0400, John Ferlan wrote:
This will grab the 'dev' from devices and do the common validation checks.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_monitor_json.c | 37 +++++++++++++++++++++---------------- 1 file changed, 21 insertions(+), 16 deletions(-)
ACK
This will fetch "this device" from the recently returned 'dev' and perform common error checking for the paths that call it. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_monitor_json.c | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 8cb3891..458b112 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -1822,6 +1822,21 @@ qemuMonitorJSONGetBlockDev(virJSONValuePtr devices, } +static const char * +qemuMonitorJSONGetBlockDevDevice(virJSONValuePtr dev) +{ + const char *thisdev; + + if (!(thisdev = virJSONValueObjectGetString(dev, "device"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("query-block device entry was not in expected format")); + return NULL; + } + + return thisdev; +} + + int qemuMonitorJSONGetBlockInfo(qemuMonitorPtr mon, virHashTablePtr table) { @@ -1842,11 +1857,8 @@ int qemuMonitorJSONGetBlockInfo(qemuMonitorPtr mon, if (!(dev = qemuMonitorJSONGetBlockDev(devices, i))) goto cleanup; - if ((thisdev = virJSONValueObjectGetString(dev, "device")) == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("block info device entry was not in expected format")); + if (!(thisdev = qemuMonitorJSONGetBlockDevDevice(dev))) goto cleanup; - } thisdev = qemuAliasDiskDriveSkipPrefix(thisdev); @@ -2102,12 +2114,8 @@ qemuMonitorJSONBlockStatsUpdateCapacity(qemuMonitorPtr mon, if (!(dev = qemuMonitorJSONGetBlockDev(devices, i))) goto cleanup; - if (!(dev_name = virJSONValueObjectGetString(dev, "device"))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("query-block device entry was not " - "in expected format")); + if (!(dev_name = qemuMonitorJSONGetBlockDevDevice(dev))) goto cleanup; - } /* drive may be empty */ if (!(inserted = virJSONValueObjectGetObject(dev, "inserted")) || @@ -4013,11 +4021,8 @@ qemuMonitorJSONDiskNameLookup(qemuMonitorPtr mon, if (!(dev = qemuMonitorJSONGetBlockDev(devices, i))) goto cleanup; - if (!(thisdev = virJSONValueObjectGetString(dev, "device"))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("block info device entry was not in expected format")); + if (!(thisdev = qemuMonitorJSONGetBlockDevDevice(dev))) goto cleanup; - } if (STREQ(thisdev, device)) { if ((inserted = virJSONValueObjectGetObject(dev, "inserted")) && -- 2.7.4
On Tue, Oct 04, 2016 at 11:28:58 -0400, John Ferlan wrote:
This will fetch "this device" from the recently returned 'dev' and perform
It fetches the device alias actually. "this device" is just a code nuance.
common error checking for the paths that call it.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_monitor_json.c | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-)
ACK
participants (2)
-
John Ferlan -
Peter Krempa