[libvirt] [PATCH v2 0/5] Combine various query-block json call paths
v1: http://www.redhat.com/archives/libvir-list/2016-September/msg01446.html NOTE: Patch 1 already ACK'd Patches 2-4 adjusted slightly to not create/use qemuMonitorJSONQueryBlockArgs instead opting to pass all the args (also shortened helper names slightly). Patch 5 adjusted to receive all args in qemuMonitorJSONQueryBlock and then call one of two callbacks based on whether the table or search is being used. I did have a version that used ATTRIBUTE_UNUSED and one generic callback, but I thought that was uglier. John Ferlan (5): qemu: Create common code for JSON "query-block" call qemu: Split out filling of JSONGetBlockInfo data qemu: Split out filling of JSONBlockStats data qemu: Split out filling of JSONDiskNameLookup data qemu: Combine the various ways to call query-block src/qemu/qemu_monitor_json.c | 356 +++++++++++++++++++++++++------------------ 1 file changed, 205 insertions(+), 151 deletions(-) For "statistical purposes" after each patch the I checked --shortstat: Patch 1: 34 insertions, 29 deletions Patch 2: 93 insertions, 68 deletions Patch 3: 122 insertions, 78 deletions Patch 4: 158 insertions, 86 deletions Patch 5: 205 insertions, 151 deletions Of the insertions, there's 53 lines of function header comments and 46 lines of function declarations... -- 2.7.4
Reduce some cut-n-paste code by creating common helper NB: This also adds error checking to qemuMonitorJSONDiskNameLookup Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_monitor_json.c | 63 ++++++++++++++++++++++++-------------------- 1 file changed, 34 insertions(+), 29 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index e1494df..4af98cc 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -1772,26 +1772,46 @@ qemuMonitorJSONSetMemoryStatsPeriod(qemuMonitorPtr mon, } +/* 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) +{ + virJSONValuePtr cmd; + virJSONValuePtr reply = NULL; + + if (!(cmd = qemuMonitorJSONMakeCommand("query-block", NULL))) + return NULL; + + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0 || + qemuMonitorJSONCheckError(cmd, reply) < 0) { + virJSONValueFree(reply); + reply = NULL; + } + + virJSONValueFree(cmd); + return reply; +} + + int qemuMonitorJSONGetBlockInfo(qemuMonitorPtr mon, virHashTablePtr table) { int ret = -1; size_t i; - virJSONValuePtr cmd = qemuMonitorJSONMakeCommand("query-block", - NULL); - virJSONValuePtr reply = NULL; + virJSONValuePtr reply; virJSONValuePtr devices; - if (!cmd) + if (!(reply = qemuMonitorJSONQueryBlock(mon))) return -1; - if (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", _("block info reply was missing device list")); @@ -1858,7 +1878,6 @@ int qemuMonitorJSONGetBlockInfo(qemuMonitorPtr mon, ret = 0; cleanup: - virJSONValueFree(cmd); virJSONValueFree(reply); return ret; } @@ -2056,21 +2075,13 @@ qemuMonitorJSONBlockStatsUpdateCapacity(qemuMonitorPtr mon, bool backingChain) { int ret = -1; - int rc; size_t i; - virJSONValuePtr cmd; - virJSONValuePtr reply = NULL; + virJSONValuePtr reply; virJSONValuePtr devices; - if (!(cmd = qemuMonitorJSONMakeCommand("query-block", NULL))) + if (!(reply = 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")); @@ -2111,7 +2122,6 @@ qemuMonitorJSONBlockStatsUpdateCapacity(qemuMonitorPtr mon, ret = 0; cleanup: - virJSONValueFree(cmd); virJSONValueFree(reply); return ret; } @@ -3987,16 +3997,12 @@ qemuMonitorJSONDiskNameLookup(qemuMonitorPtr mon, virStorageSourcePtr target) { char *ret = NULL; - virJSONValuePtr cmd = NULL; - virJSONValuePtr reply = NULL; + virJSONValuePtr reply; virJSONValuePtr devices; size_t i; - cmd = qemuMonitorJSONMakeCommand("query-block", NULL); - if (!cmd) + if (!(reply = qemuMonitorJSONQueryBlock(mon))) return NULL; - if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) - goto cleanup; if (!(devices = virJSONValueObjectGetArray(reply, "return"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -4038,7 +4044,6 @@ qemuMonitorJSONDiskNameLookup(qemuMonitorPtr mon, device); cleanup: - virJSONValueFree(cmd); virJSONValueFree(reply); return ret; -- 2.7.4
Create a helper function which will read the device data and fill in a hash table entry with that data. In preparation for some more code reuse. The helper function will take as an argument a new local static structure in order to define the arguments. This will make it possible to combine more code in later patches. NB: Adjust error reporting to not use 'removable' or 'locked' as an argument to reporting error - rather just inline that in the message. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_monitor_json.c | 98 ++++++++++++++++++++++++++------------------ 1 file changed, 59 insertions(+), 39 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 4af98cc..a12029a 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -1800,6 +1800,64 @@ qemuMonitorJSONQueryBlock(qemuMonitorPtr mon) } +/* Allocate a qemuDomainDiskInfo structure, place it into the hash table, + * parse the device, and fill in the details for the info + * + * Returns -1 on failure, 0 on success + */ +static int +qemuMonitorJSONQueryBlockFillInfo(virJSONValuePtr dev, + const char *thisdev, + virHashTablePtr table) +{ + int ret = -1; + struct qemuDomainDiskInfo *info; + const char *status; + + thisdev = qemuAliasDiskDriveSkipPrefix(thisdev); + + if (VIR_ALLOC(info) < 0) + goto cleanup; + + if (virHashAddEntry(table, thisdev, info) < 0) { + VIR_FREE(info); + goto cleanup; + } + + if (virJSONValueObjectGetBoolean(dev, "removable", &info->removable) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot read 'removable' value")); + goto cleanup; + } + + if (virJSONValueObjectGetBoolean(dev, "locked", &info->locked) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot read 'locked' value")); + goto cleanup; + } + + /* 'tray_open' is present only if the device has a tray */ + if (virJSONValueObjectGetBoolean(dev, "tray_open", &info->tray_open) == 0) + info->tray = true; + + /* presence of 'inserted' notifies that a medium is in the device */ + if (!virJSONValueObjectGetObject(dev, "inserted")) + info->empty = true; + + /* Missing io-status indicates no error */ + if ((status = virJSONValueObjectGetString(dev, "io-status"))) { + info->io_status = qemuMonitorBlockIOStatusToError(status); + if (info->io_status < 0) + goto cleanup; + } + + ret = 0; + + cleanup: + return ret; +} + + int qemuMonitorJSONGetBlockInfo(qemuMonitorPtr mon, virHashTablePtr table) { @@ -1820,9 +1878,7 @@ int qemuMonitorJSONGetBlockInfo(qemuMonitorPtr mon, for (i = 0; i < virJSONValueArraySize(devices); i++) { virJSONValuePtr dev = virJSONValueArrayGet(devices, i); - struct qemuDomainDiskInfo *info; const char *thisdev; - const char *status; if (!dev || dev->type != VIR_JSON_TYPE_OBJECT) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -1836,44 +1892,8 @@ int qemuMonitorJSONGetBlockInfo(qemuMonitorPtr mon, goto cleanup; } - thisdev = qemuAliasDiskDriveSkipPrefix(thisdev); - - if (VIR_ALLOC(info) < 0) - goto cleanup; - - if (virHashAddEntry(table, thisdev, info) < 0) { - VIR_FREE(info); - goto cleanup; - } - - if (virJSONValueObjectGetBoolean(dev, "removable", &info->removable) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot read %s value"), - "removable"); - goto cleanup; - } - - if (virJSONValueObjectGetBoolean(dev, "locked", &info->locked) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot read %s value"), - "locked"); + if (qemuMonitorJSONQueryBlockFillInfo(dev, thisdev, table) < 0) goto cleanup; - } - - /* 'tray_open' is present only if the device has a tray */ - if (virJSONValueObjectGetBoolean(dev, "tray_open", &info->tray_open) == 0) - info->tray = true; - - /* presence of 'inserted' notifies that a medium is in the device */ - if (!virJSONValueObjectGetObject(dev, "inserted")) - info->empty = true; - - /* Missing io-status indicates no error */ - if ((status = virJSONValueObjectGetString(dev, "io-status"))) { - info->io_status = qemuMonitorBlockIOStatusToError(status); - if (info->io_status < 0) - goto cleanup; - } } ret = 0; -- 2.7.4
Create a helper function which will read the device data and fill in a hash table entry with that data. In preparation for some more code reuse. Add the backingChain argument to the args structure. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_monitor_json.c | 40 ++++++++++++++++++++++++++++++---------- 1 file changed, 30 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index a12029a..f2589fc 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -2089,6 +2089,34 @@ qemuMonitorJSONBlockStatsUpdateCapacityOne(virJSONValuePtr image, } +/* Ensure the query block has something in the drive and if so make the call + * to update the stats capacity. + * + * Returns -1 on failure, 0 on success + */ +static int +qemuMonitorJSONQueryBlockFillStats(virJSONValuePtr dev, + const char *thisdev, + virHashTablePtr table, + bool backingChain) +{ + virJSONValuePtr inserted; + virJSONValuePtr image; + + /* drive may be empty */ + if (!(inserted = virJSONValueObjectGetObject(dev, "inserted")) || + !(image = virJSONValueObjectGetObject(inserted, "image"))) + return 0; + + if (qemuMonitorJSONBlockStatsUpdateCapacityOne(image, thisdev, 0, + table, + backingChain) < 0) + return -1; + + return 0; +} + + int qemuMonitorJSONBlockStatsUpdateCapacity(qemuMonitorPtr mon, virHashTablePtr stats, @@ -2110,8 +2138,6 @@ qemuMonitorJSONBlockStatsUpdateCapacity(qemuMonitorPtr mon, for (i = 0; i < virJSONValueArraySize(devices); i++) { virJSONValuePtr dev = virJSONValueArrayGet(devices, i); - virJSONValuePtr inserted; - virJSONValuePtr image; const char *dev_name; if (!dev || dev->type != VIR_JSON_TYPE_OBJECT) { @@ -2128,14 +2154,8 @@ qemuMonitorJSONBlockStatsUpdateCapacity(qemuMonitorPtr mon, goto cleanup; } - /* drive may be empty */ - if (!(inserted = virJSONValueObjectGetObject(dev, "inserted")) || - !(image = virJSONValueObjectGetObject(inserted, "image"))) - continue; - - if (qemuMonitorJSONBlockStatsUpdateCapacityOne(image, dev_name, 0, - stats, - backingChain) < 0) + if (qemuMonitorJSONQueryBlockFillStats(dev, dev_name, stats, + backingChain) < 0) goto cleanup; } -- 2.7.4
Create a helper function which will do the primary work required to find the specific device and get the disk name. Done in preparation for some more code reuse. Add new elements to _qemuMonitorJSONQueryBlockArgs in order to handle the lookup of just one specific device from the query-block code. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_monitor_json.c | 43 +++++++++++++++++++++++++++++++++++-------- 1 file changed, 35 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index f2589fc..2b03f86 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4030,6 +4030,34 @@ qemuMonitorJSONDiskNameLookupOne(virJSONValuePtr image, } +/* If the current device (thisdev) is the one we're looking for (searchDevice), + * then call LookupOne returning the desired name. + * + * Returns 0 on not found, 1 when thisdev and searchDevice match - it is up + * to the caller to determine whether @foundDevice is filled in + */ +static int +qemuMonitorJSONQueryBlockFillDiskNameLookup(virJSONValuePtr dev, + const char *thisdev, + const char *searchDevice, + virStorageSourcePtr top, + virStorageSourcePtr target, + char **foundDevice) +{ + virJSONValuePtr inserted; + virJSONValuePtr image; + + if (STREQ(thisdev, searchDevice)) { + if ((inserted = virJSONValueObjectGetObject(dev, "inserted")) && + (image = virJSONValueObjectGetObject(inserted, "image"))) { + *foundDevice = qemuMonitorJSONDiskNameLookupOne(image, top, target); + } + return 1; + } + return 0; +} + + char * qemuMonitorJSONDiskNameLookup(qemuMonitorPtr mon, const char *device, @@ -4052,9 +4080,8 @@ qemuMonitorJSONDiskNameLookup(qemuMonitorPtr mon, for (i = 0; i < virJSONValueArraySize(devices); i++) { virJSONValuePtr dev = virJSONValueArrayGet(devices, i); - virJSONValuePtr inserted; - virJSONValuePtr image; const char *thisdev; + int rc; if (!dev || dev->type != VIR_JSON_TYPE_OBJECT) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -4068,13 +4095,13 @@ qemuMonitorJSONDiskNameLookup(qemuMonitorPtr mon, goto cleanup; } - if (STREQ(thisdev, device)) { - if ((inserted = virJSONValueObjectGetObject(dev, "inserted")) && - (image = virJSONValueObjectGetObject(inserted, "image"))) { - ret = qemuMonitorJSONDiskNameLookupOne(image, top, target); - } + if ((rc = qemuMonitorJSONQueryBlockFillDiskNameLookup(dev, thisdev, + device, top, + target, + &ret)) < 0) + goto cleanup; + if (rc == 1) break; - } } /* Guarantee an error when returning NULL, but don't override a * more specific error if one was already generated. */ -- 2.7.4
Now that each of the 3 callers to "query-block" has relatively the same flow, combine the remaining flow into qemuMonitorJSONQueryBlock and of course rework the function comments to describe the expectations. The qemuMonitorJSONQueryBlock will now take multiple parameters and call the helper that exists to fill just the specific data requested. Each of the callers is essentially reduced to a shim calling the common function with its specific qemuMonitorJSONQueryBlockFill* helper to handle the needs for the specific caller. NB: Also adjust the first "error message in the devices parsing loop to make it easier to distinguish which failure occurred. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_monitor_json.c | 242 ++++++++++++++++++++----------------------- 1 file changed, 112 insertions(+), 130 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 2b03f86..e7dec88 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -129,6 +129,20 @@ static qemuEventHandler eventHandlers[] = { /* We use bsearch, so keep this list sorted. */ }; +typedef int +(*qemuMonitorJSONQueryBlockFillTableCallback)(virJSONValuePtr dev, + const char *thisdev, + virHashTablePtr table, + bool backingChain); + +typedef int +(*qemuMonitorJSONQueryBlockFillSearchCallback)(virJSONValuePtr dev, + const char *thisdev, + const char *searchDevice, + virStorageSourcePtr top, + virStorageSourcePtr target, + char **foundDevice); + static int qemuMonitorEventCompare(const void *key, const void *elt) { @@ -1774,29 +1788,107 @@ qemuMonitorJSONSetMemoryStatsPeriod(qemuMonitorPtr mon, /* qemuMonitorJSONQueryBlock: * @mon: Monitor pointer + * @table: The hash table pointer to insert a device into + * @backingChain: Boolean whether to follow backing chaing + * @searchDevice: Specific device name to search on + * @top: Top of a chain to start looking + * @target: The backing element target + * @foundDevice: Pointer to address of returned found device string + * @tablecb: Callback function for hash table usage + * @searchcb: Callback function for searchDevice search + * + * This helper will attempt to make a "query-block" call, check for + * errors, and scan the reply list of devices calling out the either the + * @tablecb or @searchcb helper in order to perform the action on the element. + * + * The function can be used in one of two ways: + * + * 1. Passing a hash @table and @tablecb in order to add each device + * in the returned list into the hash table. This option also provides + * the capability to pass a @backingChain boolean to search the backing + * chains as well. + * + * 2. Passing a @searchDevice and @searchcb in order to search the returned + * data for a specific device and then call the @searchcb with the @top + * and @target parameters in order to return a @foundDevice. * - * This helper will attempt to make a "query-block" call and check for - * errors before returning with the reply. + * The @tablecb program is expected to return -1 for error and quit, 0 for + * good status and continue. * - * Returns: NULL on error, reply on success + * The @searchcb program is expected to return -1 for error and quit, 0 for + * good status and continue, and 1 for good status and return. The caller + * for usage of @foundDevice is expected to pass a non-NULL argument. + * + * Returns: -1 on error, 0 on success */ -static virJSONValuePtr -qemuMonitorJSONQueryBlock(qemuMonitorPtr mon) +static int +qemuMonitorJSONQueryBlock(qemuMonitorPtr mon, + virHashTablePtr table, + bool backingChain, + const char *searchDevice, + virStorageSourcePtr top, + virStorageSourcePtr target, + char **foundDevice, + qemuMonitorJSONQueryBlockFillTableCallback tablecb, + qemuMonitorJSONQueryBlockFillSearchCallback searchcb) { + int ret = -1; + size_t i; virJSONValuePtr cmd; virJSONValuePtr reply = NULL; + virJSONValuePtr devices; if (!(cmd = qemuMonitorJSONMakeCommand("query-block", NULL))) - return NULL; + return -1; if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0 || - qemuMonitorJSONCheckError(cmd, reply) < 0) { - virJSONValueFree(reply); - reply = NULL; + qemuMonitorJSONCheckError(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; + const char *thisdev; + int rc; + + dev = virJSONValueArrayGet(devices, i); + if (!dev || dev->type != VIR_JSON_TYPE_OBJECT) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("block info device or type was not in " + "expected format")); + goto cleanup; + } + + if (!(thisdev = virJSONValueObjectGetString(dev, "device"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("block info device entry was not in " + "expected format")); + goto cleanup; + } + + if (tablecb) { + if (tablecb(dev, thisdev, table, backingChain) < 0) + goto cleanup; + } else if (searchcb) { + if ((rc = searchcb(dev, thisdev, searchDevice, top, + target, foundDevice) < 0)) + goto cleanup; + + if (rc == 1) + break; + } } + ret = 0; + cleanup: + virJSONValueFree(reply); virJSONValueFree(cmd); - return reply; + return ret; } @@ -1808,7 +1900,8 @@ qemuMonitorJSONQueryBlock(qemuMonitorPtr mon) static int qemuMonitorJSONQueryBlockFillInfo(virJSONValuePtr dev, const char *thisdev, - virHashTablePtr table) + virHashTablePtr table, + bool backingChain ATTRIBUTE_UNUSED) { int ret = -1; struct qemuDomainDiskInfo *info; @@ -1861,45 +1954,8 @@ qemuMonitorJSONQueryBlockFillInfo(virJSONValuePtr dev, int qemuMonitorJSONGetBlockInfo(qemuMonitorPtr mon, virHashTablePtr table) { - int ret = -1; - size_t i; - - virJSONValuePtr reply; - virJSONValuePtr devices; - - if (!(reply = qemuMonitorJSONQueryBlock(mon))) - return -1; - - 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); - 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")); - goto cleanup; - } - - if ((thisdev = virJSONValueObjectGetString(dev, "device")) == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("block info device entry was not in expected format")); - goto cleanup; - } - - if (qemuMonitorJSONQueryBlockFillInfo(dev, thisdev, table) < 0) - goto cleanup; - } - - ret = 0; - cleanup: - virJSONValueFree(reply); - return ret; + return qemuMonitorJSONQueryBlock(mon, table, false, NULL, NULL, NULL, NULL, + qemuMonitorJSONQueryBlockFillInfo, NULL); } @@ -2122,48 +2178,9 @@ qemuMonitorJSONBlockStatsUpdateCapacity(qemuMonitorPtr mon, virHashTablePtr stats, bool backingChain) { - int ret = -1; - size_t i; - virJSONValuePtr reply; - virJSONValuePtr devices; - - if (!(reply = qemuMonitorJSONQueryBlock(mon))) - return -1; - - 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); - 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")); - goto cleanup; - } - - if (!(dev_name = virJSONValueObjectGetString(dev, "device"))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("query-block device entry was not " - "in expected format")); - goto cleanup; - } - - if (qemuMonitorJSONQueryBlockFillStats(dev, dev_name, stats, - backingChain) < 0) - goto cleanup; - } - - ret = 0; - - cleanup: - virJSONValueFree(reply); - return ret; + return qemuMonitorJSONQueryBlock(mon, stats, backingChain, + NULL, NULL, NULL, NULL, + qemuMonitorJSONQueryBlockFillStats, NULL); } @@ -4065,44 +4082,12 @@ qemuMonitorJSONDiskNameLookup(qemuMonitorPtr mon, virStorageSourcePtr target) { char *ret = NULL; - virJSONValuePtr reply; - virJSONValuePtr devices; - size_t i; - if (!(reply = qemuMonitorJSONQueryBlock(mon))) + if (qemuMonitorJSONQueryBlock(mon, NULL, false, + device, top, target, &ret, NULL, + qemuMonitorJSONQueryBlockFillDiskNameLookup) < 0) return NULL; - 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); - const char *thisdev; - int rc; - - if (!dev || dev->type != VIR_JSON_TYPE_OBJECT) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("block info device entry was not in expected format")); - goto cleanup; - } - - if (!(thisdev = virJSONValueObjectGetString(dev, "device"))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("block info device entry was not in expected format")); - goto cleanup; - } - - if ((rc = qemuMonitorJSONQueryBlockFillDiskNameLookup(dev, thisdev, - device, top, - target, - &ret)) < 0) - goto cleanup; - if (rc == 1) - break; - } /* Guarantee an error when returning NULL, but don't override a * more specific error if one was already generated. */ if (!ret && !virGetLastError()) @@ -4110,9 +4095,6 @@ qemuMonitorJSONDiskNameLookup(qemuMonitorPtr mon, _("unable to find backing name for device %s"), device); - cleanup: - virJSONValueFree(reply); - return ret; } -- 2.7.4
On Fri, Sep 30, 2016 at 14:39:16 -0400, John Ferlan wrote:
v1: http://www.redhat.com/archives/libvir-list/2016-September/msg01446.html
NOTE: Patch 1 already ACK'd
Patches 2-4 adjusted slightly to not create/use qemuMonitorJSONQueryBlockArgs instead opting to pass all the args (also shortened helper names slightly).
Patch 5 adjusted to receive all args in qemuMonitorJSONQueryBlock and then call one of two callbacks based on whether the table or search is being used. I did have a version that used ATTRIBUTE_UNUSED and one generic callback, but I thought that was uglier.
It is quite ugly, but that does not differ much from v1. At least with this versions you don't have to jump around to find the arguments for the callbacks. As I've said in v1 review I don't quite see a value in introducing callbacks just to be able to extract 3 repetitions of a for loop. Especially since the worker functions take pretty diverse parameters. What would make sense to extract is the checks that validate that the returned data is in somewhat sane format in the helper that you've added in patch 1. (also mentioned in previous review). NACK to 2 - 5. Peter
On 10/03/2016 03:22 AM, Peter Krempa wrote:
On Fri, Sep 30, 2016 at 14:39:16 -0400, John Ferlan wrote:
v1: http://www.redhat.com/archives/libvir-list/2016-September/msg01446.html
NOTE: Patch 1 already ACK'd
Patches 2-4 adjusted slightly to not create/use qemuMonitorJSONQueryBlockArgs instead opting to pass all the args (also shortened helper names slightly).
Patch 5 adjusted to receive all args in qemuMonitorJSONQueryBlock and then call one of two callbacks based on whether the table or search is being used. I did have a version that used ATTRIBUTE_UNUSED and one generic callback, but I thought that was uglier.
It is quite ugly, but that does not differ much from v1. At least with this versions you don't have to jump around to find the arguments for the callbacks.
As I've said in v1 review I don't quite see a value in introducing callbacks just to be able to extract 3 repetitions of a for loop. Especially since the worker functions take pretty diverse parameters.
And as I said in my response - it's primarily to cut down on cut-n-paste (code repetition) of the 3 methods that are parsing the same returned data in the same way, but looking for different things. Two of the workers take essentially the same parameters (one takes backingChain) while the third worker is quite different, but that's because it's looking for more specific data. As also noted in my response, it's the PITA. The goal/purpose was a reaction to changes proposed in the introduce pull backups patch series which modified the returned GetBlockInfo to add a fetch/store of 'virtual-size'. Those changes introduced some error logic into a function that concerned me. IIRC, the logic took what could be a non static value and treated it as static. So rather than fetch the value just once at startup, the thought was introduce a means that would allow "new code" to fetch what was desired as it was needed. In fact, I think my response indicated that code should mimic how the FillDiskNameLookup code works although perhaps misinterpreted to being add the virtual-size field to that code.
What would make sense to extract is the checks that validate that the returned data is in somewhat sane format in the helper that you've added in patch 1. (also mentioned in previous review).
Since you didn't explicitly state which checks, I'm assuming you mean either: if (!(devices = virJSONValueObjectGetArray(reply, "return"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("block info reply was missing device list")); goto cleanup; } I did consider moving this into the helper and returning devices but ran into the problem that the helper would virJSONValueFree(reply) while returning devices which is then used by the for loop to get each device (dev = virJSONValueArrayGet(devices, i)). Since devices points into reply, I didn't think that would be such a good idea to free reply and then return devices. Returning reply and fetching devices again seemed pointless. or you could also be referencing the for loop checks : if (!dev || dev->type != VIR_JSON_TYPE_OBJECT) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("block info device entry was not in expected format")); goto cleanup; } if (!(thisdev = virJSONValueObjectGetString(dev, "device"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("block info device entry was not in expected format")); goto cleanup; } I see a very nominally small value in creating a helper to do those checks. In any case, I can drop patches 2-5. I do see value in them, but also understand your point about the callbacks and arguments. John
On Mon, Oct 03, 2016 at 07:49:44 -0400, John Ferlan wrote:
On 10/03/2016 03:22 AM, Peter Krempa wrote:
On Fri, Sep 30, 2016 at 14:39:16 -0400, John Ferlan wrote:
v1: http://www.redhat.com/archives/libvir-list/2016-September/msg01446.html
NOTE: Patch 1 already ACK'd
Patches 2-4 adjusted slightly to not create/use qemuMonitorJSONQueryBlockArgs instead opting to pass all the args (also shortened helper names slightly).
Patch 5 adjusted to receive all args in qemuMonitorJSONQueryBlock and then call one of two callbacks based on whether the table or search is being used. I did have a version that used ATTRIBUTE_UNUSED and one generic callback, but I thought that was uglier.
It is quite ugly, but that does not differ much from v1. At least with this versions you don't have to jump around to find the arguments for the callbacks.
As I've said in v1 review I don't quite see a value in introducing callbacks just to be able to extract 3 repetitions of a for loop. Especially since the worker functions take pretty diverse parameters.
And as I said in my response - it's primarily to cut down on cut-n-paste (code repetition) of the 3 methods that are parsing the same returned data in the same way, but looking for different things.
Two of the workers take essentially the same parameters (one takes backingChain) while the third worker is quite different, but that's because it's looking for more specific data. As also noted in my response, it's the PITA.
That should be perhaps a hint that they should not be merged.
The goal/purpose was a reaction to changes proposed in the introduce pull backups patch series which modified the returned GetBlockInfo to add a fetch/store of 'virtual-size'. Those changes introduced some error logic into a function that concerned me. IIRC, the logic took what could be a non static value and treated it as static. So rather than fetch the value just once at startup, the thought was introduce a means that would allow "new code" to fetch what was desired as it was needed. In fact, I think my response indicated that code should mimic how the FillDiskNameLookup code works although perhaps misinterpreted to being add the virtual-size field to that code.
If it's a problem to add it to the current structures there are two options: 1) The design of the new code is bad 2) The design of the libvirt code currently in is bad Looking at the code, the design of the feature is bad. The checks don't necessarily need to be in the monitor extractor, but the usage place can check that the required data is present and valid. Currently the libvirt block job code is not really how it should be. We are not able to properly track the bakcing chains in the XML and map them to qemu block information nor are we using node names. Until we fix this all block job code won't work properly Either way we should not try to find ways how to hack it around but make it properly. I plan to finally get around and fix all the block job and backing chain problems we have after I finish with the vcpu stuff so I'd really appreciate if I don't have to remove hacks like this in the near future.
What would make sense to extract is the checks that validate that the returned data is in somewhat sane format in the helper that you've added in patch 1. (also mentioned in previous review).
Since you didn't explicitly state which checks, I'm assuming you mean either:
if (!(devices = virJSONValueObjectGetArray(reply, "return"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("block info reply was missing device list")); goto cleanup; }
I did consider moving this into the helper and returning devices but ran into the problem that the helper would virJSONValueFree(reply) while returning devices which is then used by the for loop to get each device (dev = virJSONValueArrayGet(devices, i)).
What's the problem wit that? Creating a function that steals the value from a JSON object should be pretty simple. We have one that steals array members: virJSONValueArraySteal.
Since devices points into reply, I didn't think that would be such a good idea to free reply and then return devices. Returning reply anda
Why not?
fetching devices again seemed pointless.
or you could also be referencing the for loop checks :
if (!dev || dev->type != VIR_JSON_TYPE_OBJECT) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("block info device entry was not in expected format")); goto cleanup; }
if (!(thisdev = virJSONValueObjectGetString(dev, "device"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("block info device entry was not in expected format")); goto cleanup; }
I see a very nominally small value in creating a helper to do those checks.
Well, that's basically the core of the duplicated code you are trying to delete ... There's not much left after that ...
In any case, I can drop patches 2-5. I do see value in them, but also understand your point about the callbacks and arguments.
I fail to see the value. This is the "duplicated" code according to your patch. Let's deconstruct it: if (!(devices = virJSONValueObjectGetArray(reply, "return"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("block info reply was missing device list")); goto cleanup; } [1] for (i = 0; i < virJSONValueArraySize(devices); i++) { [2] virJSONValuePtr dev; const char *thisdev; int rc; dev = virJSONValueArrayGet(devices, i); [3] if (!dev || dev->type != VIR_JSON_TYPE_OBJECT) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("block info device or type was not in " "expected format")); goto cleanup; } [4] if (!(thisdev = virJSONValueObjectGetString(dev, "device"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("block info device entry was not in " "expected format")); goto cleanup; } [5] So let's tear it down piece by piece: [1] A global check which can easily be extracted after patch 1 into the common function. [4] If this check bothers you, for a simple expense of adding a for loop this can be part of the common code calling the monitor command itself. One iteration over a rather short array, where you can make sure that the returned JSON array is valid. [5] This extracts the name of the device. A simple static function can replace it and report the proper error. if (!(thisdev = qemuMonitorQueryBlockGetDevice(dev))) goto cleanup; [2] and [3] This very complex (sarcasm) structure loops over the entries and extracts individual members. I really fail to see how extracting this at the price of callbacks and/or arguments hidden in a structure make the code any better or any further expansions simpler. Peter
[...]
Since you didn't explicitly state which checks, I'm assuming you mean either:
if (!(devices = virJSONValueObjectGetArray(reply, "return"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("block info reply was missing device list")); goto cleanup; }
I did consider moving this into the helper and returning devices but ran into the problem that the helper would virJSONValueFree(reply) while returning devices which is then used by the for loop to get each device (dev = virJSONValueArrayGet(devices, i)).
What's the problem wit that? Creating a function that steals the value from a JSON object should be pretty simple. We have one that steals array members: virJSONValueArraySteal.
Since devices points into reply, I didn't think that would be such a good idea to free reply and then return devices. Returning reply anda
Why not?
Let's assume the helper is: +static virJSONValuePtr +qemuMonitorJSONQueryBlock(qemuMonitorPtr mon) +{ + virJSONValuePtr cmd; + virJSONValuePtr reply = NULL; + virJSONValuePtr devices = NULL; + + if (!(cmd = qemuMonitorJSONMakeCommand("query-block", NULL))) + return NULL; + + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0 || + 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; + } + + cleanup: + virJSONValueFree(cmd); + virJSONValueFree(reply); + return devices; +} Wouldn't "reply" being free'd cause access problems for devices? Is there other code somewhere else that gets the array, free's the object, then parses the array? I didn't find any (for good reason)... Sure we could still return reply... Then back in the callers make the same virJSONValueObjectGetArray call without the error check to get devices, but I didn't see the value in doing so. Returning both reply and devices is possible, but similar such uses in the past haven't been wildly popular. And yes, I figured out what the code does. We differ in opinion on the value of combining the duplicated code. Since you plan on making further adjustments in this space, then fine as I've already pointed out I'll drop patches 2-5. John
fetching devices again seemed pointless.
or you could also be referencing the for loop checks :
if (!dev || dev->type != VIR_JSON_TYPE_OBJECT) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("block info device entry was not in expected format")); goto cleanup; }
if (!(thisdev = virJSONValueObjectGetString(dev, "device"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("block info device entry was not in expected format")); goto cleanup; }
I see a very nominally small value in creating a helper to do those checks.
Well, that's basically the core of the duplicated code you are trying to delete ... There's not much left after that ...
In any case, I can drop patches 2-5. I do see value in them, but also understand your point about the callbacks and arguments.
I fail to see the value. This is the "duplicated" code according to your patch. Let's deconstruct it:
if (!(devices = virJSONValueObjectGetArray(reply, "return"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("block info reply was missing device list")); goto cleanup; } [1]
for (i = 0; i < virJSONValueArraySize(devices); i++) { [2] virJSONValuePtr dev; const char *thisdev; int rc;
dev = virJSONValueArrayGet(devices, i); [3] if (!dev || dev->type != VIR_JSON_TYPE_OBJECT) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("block info device or type was not in " "expected format")); goto cleanup; } [4]
if (!(thisdev = virJSONValueObjectGetString(dev, "device"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("block info device entry was not in " "expected format")); goto cleanup; } [5]
So let's tear it down piece by piece: [1] A global check which can easily be extracted after patch 1 into the common function.
[4] If this check bothers you, for a simple expense of adding a for loop this can be part of the common code calling the monitor command itself. One iteration over a rather short array, where you can make sure that the returned JSON array is valid.
[5] This extracts the name of the device. A simple static function can replace it and report the proper error. if (!(thisdev = qemuMonitorQueryBlockGetDevice(dev))) goto cleanup;
[2] and [3] This very complex (sarcasm) structure loops over the entries and extracts individual members. I really fail to see how extracting this at the price of callbacks and/or arguments hidden in a structure make the code any better or any further expansions simpler.
Peter
On Mon, Oct 03, 2016 at 10:46:07 -0400, John Ferlan wrote:
[...]
Since you didn't explicitly state which checks, I'm assuming you mean either:
if (!(devices = virJSONValueObjectGetArray(reply, "return"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("block info reply was missing device list")); goto cleanup; }
I did consider moving this into the helper and returning devices but ran into the problem that the helper would virJSONValueFree(reply) while returning devices which is then used by the for loop to get each device (dev = virJSONValueArrayGet(devices, i)).
What's the problem wit that? Creating a function that steals the value from a JSON object should be pretty simple. We have one that steals array members: virJSONValueArraySteal.
Since devices points into reply, I didn't think that would be such a good idea to free reply and then return devices. Returning reply anda
Why not?
Let's assume the helper is:
+static virJSONValuePtr +qemuMonitorJSONQueryBlock(qemuMonitorPtr mon) +{ + virJSONValuePtr cmd; + virJSONValuePtr reply = NULL; + virJSONValuePtr devices = NULL; + + if (!(cmd = qemuMonitorJSONMakeCommand("query-block", NULL))) + return NULL; + + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0 || + 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; + } + + cleanup: + virJSONValueFree(cmd); + virJSONValueFree(reply); + return devices; +}
Wouldn't "reply" being free'd cause access problems for devices? Is there other code somewhere else that gets the array, free's the object, then parses the array? I didn't find any (for good reason)...
It would as virJSONValueFree is recursive and thus everything would be freed. Hence my comment of adding the "steal" function.
On 10/03/2016 10:49 AM, Peter Krempa wrote:
On Mon, Oct 03, 2016 at 10:46:07 -0400, John Ferlan wrote:
[...]
Since you didn't explicitly state which checks, I'm assuming you mean either:
if (!(devices = virJSONValueObjectGetArray(reply, "return"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("block info reply was missing device list")); goto cleanup; }
I did consider moving this into the helper and returning devices but ran into the problem that the helper would virJSONValueFree(reply) while returning devices which is then used by the for loop to get each device (dev = virJSONValueArrayGet(devices, i)).
What's the problem wit that? Creating a function that steals the value from a JSON object should be pretty simple. We have one that steals array members: virJSONValueArraySteal.
Since devices points into reply, I didn't think that would be such a good idea to free reply and then return devices. Returning reply anda
Why not?
Let's assume the helper is:
+static virJSONValuePtr +qemuMonitorJSONQueryBlock(qemuMonitorPtr mon) +{ + virJSONValuePtr cmd; + virJSONValuePtr reply = NULL; + virJSONValuePtr devices = NULL; + + if (!(cmd = qemuMonitorJSONMakeCommand("query-block", NULL))) + return NULL; + + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0 || + 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; + } + + cleanup: + virJSONValueFree(cmd); + virJSONValueFree(reply); + return devices; +}
Wouldn't "reply" being free'd cause access problems for devices? Is there other code somewhere else that gets the array, free's the object, then parses the array? I didn't find any (for good reason)...
It would as virJSONValueFree is recursive and thus everything would be freed. Hence my comment of adding the "steal" function.
I find it ironic that it's more desirable to add code to virjson.c (1 external and 2 static), virjson.h, and libvirt.private_syms for the single purpose that it's more desirable to have 3 API's follow the same steps and it's less desirable to combine those API's in some manner. We have a few API's that will pass/receive many parameters including multiple NULL's and make the proper decision based on some key for the function. We have a difference of opinion on the way to do this - that's fine. I'm moving on. No sense in beating this dead horse anymore. John
participants (2)
-
John Ferlan -
Peter Krempa