[libvirt] [PATCH 0/5] Combine various query-block json call paths
Based off a recent review and discussion regarding adding a new field to qemuDomainDiskInfo: http://www.redhat.com/archives/libvir-list/2016-September/msg00193.html I decided to take the plunge and rework the query-block code in order to combine the various ways callers return the data and well hopefully allow for an easier "future mechanism" to call the query-block returning just the required/requested data for any caller (with a little bit of code wrangling). 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 | 345 ++++++++++++++++++++++++------------------- 1 file changed, 193 insertions(+), 152 deletions(-) -- 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
On Fri, Sep 30, 2016 at 08:53:50 -0400, John Ferlan wrote:
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(-)
ACK (after release) Peter
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 | 117 +++++++++++++++++++++++++++---------------- 1 file changed, 74 insertions(+), 43 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 4af98cc..58e6105 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -129,6 +129,14 @@ static qemuEventHandler eventHandlers[] = { /* We use bsearch, so keep this list sorted. */ }; +typedef struct _qemuMonitorJSONQueryBlockArgs qemuMonitorJSONQueryBlockArgs; +typedef qemuMonitorJSONQueryBlockArgs *qemuMonitorJSONQueryBlockArgsPtr; +struct _qemuMonitorJSONQueryBlockArgs { + virJSONValuePtr dev; + virHashTablePtr table; + const char *thisdev; +}; + static int qemuMonitorEventCompare(const void *key, const void *elt) { @@ -1800,6 +1808,66 @@ qemuMonitorJSONQueryBlock(qemuMonitorPtr mon) } +/* Taking a query block argument, allocate a qemuDomainDiskInfo structure, + * place it into the args.table hash table, and then fill in the various + * fields of the info structure that are pertinent. + * + * Returns -1 on failure, 0 on success + */ +static int +qemuMonitorJSONQueryBlockFillBlockInfoTable(qemuMonitorJSONQueryBlockArgsPtr args) +{ + int ret = -1; + struct qemuDomainDiskInfo *info; + const char *status; + const char *thisdev; + + thisdev = qemuAliasDiskDriveSkipPrefix(args->thisdev); + + if (VIR_ALLOC(info) < 0) + goto cleanup; + + if (virHashAddEntry(args->table, thisdev, info) < 0) { + VIR_FREE(info); + goto cleanup; + } + + if (virJSONValueObjectGetBoolean(args->dev, "removable", + &info->removable) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot read 'removable' value")); + goto cleanup; + } + + if (virJSONValueObjectGetBoolean(args->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(args->dev, "tray_open", + &info->tray_open) == 0) + info->tray = true; + + /* presence of 'inserted' notifies that a medium is in the device */ + if (!virJSONValueObjectGetObject(args->dev, "inserted")) + info->empty = true; + + /* Missing io-status indicates no error */ + if ((status = virJSONValueObjectGetString(args->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) { @@ -1819,61 +1887,24 @@ 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; + qemuMonitorJSONQueryBlockArgs args = {0}; - if (!dev || dev->type != VIR_JSON_TYPE_OBJECT) { + args.dev = virJSONValueArrayGet(devices, i); + if (!args.dev || args.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) { + if (!(args.thisdev = virJSONValueObjectGetString(args.dev, "device"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("block info device entry was not in expected format")); 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"); + args.table = table; + if (qemuMonitorJSONQueryBlockFillBlockInfoTable(&args) < 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 | 47 +++++++++++++++++++++++++++++++------------- 1 file changed, 33 insertions(+), 14 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 58e6105..db60ed4 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -135,6 +135,7 @@ struct _qemuMonitorJSONQueryBlockArgs { virJSONValuePtr dev; virHashTablePtr table; const char *thisdev; + bool backingChain; }; static int @@ -2100,6 +2101,31 @@ qemuMonitorJSONBlockStatsUpdateCapacityOne(virJSONValuePtr image, } +/* Taking a query block argument, ensure the query block has something in + * the drive and if so make the call to fill update the stats capacity. + * + * Returns -1 on failure, 0 on success + */ +static int +qemuMonitorJSONQueryBlockFillBlockStatsTable(qemuMonitorJSONQueryBlockArgsPtr args) +{ + virJSONValuePtr inserted; + virJSONValuePtr image; + + /* drive may be empty */ + if (!(inserted = virJSONValueObjectGetObject(args->dev, "inserted")) || + !(image = virJSONValueObjectGetObject(inserted, "image"))) + return 0; + + if (qemuMonitorJSONBlockStatsUpdateCapacityOne(image, args->thisdev, 0, + args->table, + args->backingChain) < 0) + return -1; + + return 0; +} + + int qemuMonitorJSONBlockStatsUpdateCapacity(qemuMonitorPtr mon, virHashTablePtr stats, @@ -2120,33 +2146,26 @@ qemuMonitorJSONBlockStatsUpdateCapacity(qemuMonitorPtr mon, } for (i = 0; i < virJSONValueArraySize(devices); i++) { - virJSONValuePtr dev = virJSONValueArrayGet(devices, i); - virJSONValuePtr inserted; - virJSONValuePtr image; - const char *dev_name; + qemuMonitorJSONQueryBlockArgs args = {0}; - if (!dev || dev->type != VIR_JSON_TYPE_OBJECT) { + args.dev = virJSONValueArrayGet(devices, i); + if (!args.dev || args.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"))) { + if (!(args.thisdev = virJSONValueObjectGetString(args.dev, "device"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("query-block device entry was not " "in expected format")); 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) + args.table = stats; + args.backingChain = backingChain; + if (qemuMonitorJSONQueryBlockFillBlockStatsTable(&args) < 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 | 52 ++++++++++++++++++++++++++++++++++---------- 1 file changed, 41 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index db60ed4..24fb397 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -136,6 +136,10 @@ struct _qemuMonitorJSONQueryBlockArgs { virHashTablePtr table; const char *thisdev; bool backingChain; + const char *searchDevice; + virStorageSourcePtr top; + virStorageSourcePtr target; + char *foundDevice; }; static int @@ -4040,6 +4044,31 @@ qemuMonitorJSONDiskNameLookupOne(virJSONValuePtr image, } +/* Taking a query block argument, if the current device (thisdev) is the + * one we're looking for (searchDevice), then call LookupDiskName. The caller + * will handle the case where the devices match, but the lookup call fails. + * + * Returns 0 on not found, 1 on found + */ +static int +qemuMonitorJSONQueryBlockDiskNameLookup(qemuMonitorJSONQueryBlockArgsPtr args) +{ + virJSONValuePtr inserted; + virJSONValuePtr image; + + if (STREQ(args->thisdev, args->searchDevice)) { + if ((inserted = virJSONValueObjectGetObject(args->dev, "inserted")) && + (image = virJSONValueObjectGetObject(inserted, "image"))) { + args->foundDevice = qemuMonitorJSONDiskNameLookupOne(image, + args->top, + args->target); + } + return 1; + } + return 0; +} + + char * qemuMonitorJSONDiskNameLookup(qemuMonitorPtr mon, const char *device, @@ -4061,28 +4090,29 @@ qemuMonitorJSONDiskNameLookup(qemuMonitorPtr mon, } for (i = 0; i < virJSONValueArraySize(devices); i++) { - virJSONValuePtr dev = virJSONValueArrayGet(devices, i); - virJSONValuePtr inserted; - virJSONValuePtr image; - const char *thisdev; + qemuMonitorJSONQueryBlockArgs args = {0}; + int rc; - if (!dev || dev->type != VIR_JSON_TYPE_OBJECT) { + args.dev = virJSONValueArrayGet(devices, i); + if (!args.dev || args.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"))) { + if (!(args.thisdev = virJSONValueObjectGetString(args.dev, "device"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("block info device entry was not in expected format")); goto cleanup; } - if (STREQ(thisdev, device)) { - if ((inserted = virJSONValueObjectGetObject(dev, "inserted")) && - (image = virJSONValueObjectGetObject(inserted, "image"))) { - ret = qemuMonitorJSONDiskNameLookupOne(image, top, target); - } + args.searchDevice = device; + args.top = top; + args.target = target; + if ((rc = qemuMonitorJSONQueryBlockDiskNameLookup(&args)) < 0) + goto cleanup; + if (rc == 1) { + ret = args.foundDevice; break; } } -- 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, "blindly" building up the qemuMonitorJSONQueryBlockArgs, and then calling the specific callback for each of the three consumers. 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 | 222 +++++++++++++++++-------------------------- 1 file changed, 89 insertions(+), 133 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 24fb397..357660a 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -142,6 +142,8 @@ struct _qemuMonitorJSONQueryBlockArgs { char *foundDevice; }; +typedef int (*qemuMonitorJSONQueryBlockFillCallback)(qemuMonitorJSONQueryBlockArgsPtr args); + static int qemuMonitorEventCompare(const void *key, const void *elt) { @@ -1787,29 +1789,97 @@ 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 + * @cb: The callback function to fill in the specific data needs + * + * This helper will attempt to make a "query-block" call, check for + * errors, and scan the reply list of devices calling out the @cb program + * in order to perform the action on the element. + * + * The function can be used in one of two ways - either passing a hash + * @table and whether to search the backing chain in order to fill in the + * hash table with all the found devices or to pass a @searchDevice, @top, + * and @target in order to search the returned data for a specific device + * and return the @foundDevice string. * - * This helper will attempt to make a "query-block" call and check for - * errors before returning with the reply. + * The @cb 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: NULL on error, reply on success + * 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, + qemuMonitorJSONQueryBlockFillCallback cb) { + 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++) { + int rc; + qemuMonitorJSONQueryBlockArgs args = {0}; + + args.dev = virJSONValueArrayGet(devices, i); + if (!args.dev || args.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 (!(args.thisdev = virJSONValueObjectGetString(args.dev, "device"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("block info device entry was not in " + "expected format")); + goto cleanup; + } + + args.table = table; + args.backingChain = backingChain; + args.searchDevice = searchDevice; + args.top = top; + args.target = target; + if ((rc = cb(&args)) < 0) + goto cleanup; + + if (rc == 1) { + *foundDevice = args.foundDevice; + break; + } } + ret = 0; + cleanup: + virJSONValueFree(reply); virJSONValueFree(cmd); - return reply; + return ret; } @@ -1876,46 +1946,9 @@ qemuMonitorJSONQueryBlockFillBlockInfoTable(qemuMonitorJSONQueryBlockArgsPtr arg 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++) { - qemuMonitorJSONQueryBlockArgs args = {0}; - - args.dev = virJSONValueArrayGet(devices, i); - if (!args.dev || args.dev->type != VIR_JSON_TYPE_OBJECT) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("block info device entry was not in expected format")); - goto cleanup; - } - - if (!(args.thisdev = virJSONValueObjectGetString(args.dev, "device"))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("block info device entry was not in expected format")); - goto cleanup; - } - - args.table = table; - if (qemuMonitorJSONQueryBlockFillBlockInfoTable(&args) < 0) - goto cleanup; - } - - ret = 0; - cleanup: - virJSONValueFree(reply); - return ret; + return qemuMonitorJSONQueryBlock(mon, table, false, + NULL, NULL, NULL, NULL, + qemuMonitorJSONQueryBlockFillBlockInfoTable); } @@ -2135,49 +2168,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++) { - qemuMonitorJSONQueryBlockArgs args = {0}; - - args.dev = virJSONValueArrayGet(devices, i); - if (!args.dev || args.dev->type != VIR_JSON_TYPE_OBJECT) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("query-block device entry was not " - "in expected format")); - goto cleanup; - } - - if (!(args.thisdev = virJSONValueObjectGetString(args.dev, "device"))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("query-block device entry was not " - "in expected format")); - goto cleanup; - } - - args.table = stats; - args.backingChain = backingChain; - if (qemuMonitorJSONQueryBlockFillBlockStatsTable(&args) < 0) - goto cleanup; - } - - ret = 0; - - cleanup: - virJSONValueFree(reply); - return ret; + return qemuMonitorJSONQueryBlock(mon, stats, backingChain, + NULL, NULL, NULL, NULL, + qemuMonitorJSONQueryBlockFillBlockStatsTable); } @@ -4076,46 +4069,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, + qemuMonitorJSONQueryBlockDiskNameLookup) < 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++) { - qemuMonitorJSONQueryBlockArgs args = {0}; - int rc; - - args.dev = virJSONValueArrayGet(devices, i); - if (!args.dev || args.dev->type != VIR_JSON_TYPE_OBJECT) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("block info device entry was not in expected format")); - goto cleanup; - } - - if (!(args.thisdev = virJSONValueObjectGetString(args.dev, "device"))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("block info device entry was not in expected format")); - goto cleanup; - } - - args.searchDevice = device; - args.top = top; - args.target = target; - if ((rc = qemuMonitorJSONQueryBlockDiskNameLookup(&args)) < 0) - goto cleanup; - if (rc == 1) { - ret = args.foundDevice; - break; - } - } /* Guarantee an error when returning NULL, but don't override a * more specific error if one was already generated. */ if (!ret && !virGetLastError()) @@ -4123,9 +4082,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 08:53:49 -0400, John Ferlan wrote:
Based off a recent review and discussion regarding adding a new field to qemuDomainDiskInfo:
http://www.redhat.com/archives/libvir-list/2016-September/msg00193.html
I decided to take the plunge and rework the query-block code in order to combine the various ways callers return the data and well hopefully allow for an easier "future mechanism" to call the query-block returning just the required/requested data for any caller (with a little bit of code wrangling).
I had a look at the patches and I'm not quite sure that you've succeeded in the goal above.
John Ferlan (5): qemu: Create common code for JSON "query-block" call
This patch is worthwhile it reduces some of the duplicated code that is necessary for all callers. Another similar cleanup could add a checker that validates that the data returned by qemu are in the expected format which was duplicated.
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
Those above are a bit hairy though. I don't see much value in not-duplicating the loop iterating the replies. I must say I don't like the arguments structure for the callback at all. It really destroys readability of the callback functions. Expanding at least the common arguments like the current device object would help massively since you don't need to look into the structure, but just the function header. As the patches progressively add more and more stuff into the callback data it really kills the advantage of using the callback at all. It's just separate functions hidden by a structure. Extracting the loop into a function really doesn't make sense.
src/qemu/qemu_monitor_json.c | 345 ++++++++++++++++++++++++------------------- 1 file changed, 193 insertions(+), 152 deletions(-)
This also indicates that it didn't really help. Peter
On 09/30/2016 09:25 AM, Peter Krempa wrote:
On Fri, Sep 30, 2016 at 08:53:49 -0400, John Ferlan wrote:
Based off a recent review and discussion regarding adding a new field to qemuDomainDiskInfo:
http://www.redhat.com/archives/libvir-list/2016-September/msg00193.html
I decided to take the plunge and rework the query-block code in order to combine the various ways callers return the data and well hopefully allow for an easier "future mechanism" to call the query-block returning just the required/requested data for any caller (with a little bit of code wrangling).
I had a look at the patches and I'm not quite sure that you've succeeded in the goal above.
John Ferlan (5): qemu: Create common code for JSON "query-block" call
This patch is worthwhile it reduces some of the duplicated code that is necessary for all callers.
Another similar cleanup could add a checker that validates that the data returned by qemu are in the expected format which was duplicated.
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
Those above are a bit hairy though. I don't see much value in not-duplicating the loop iterating the replies.
To me the advantage was from the aspect of cut-copy-paste to pull out devices and then walk it doing the same error checking for all 3 methods, but I certainly understand your point. The PITA is DiskNameLookup which has different args and wanting to exit as soon as something is found. I did consider making a 1 entry hash table and passing a bool to quit when something was found, but didn't think it would be worth the effort... I can look into that option again.
I must say I don't like the arguments structure for the callback at all. It really destroys readability of the callback functions. Expanding at least the common arguments like the current device object would help massively since you don't need to look into the structure, but just the function header.
While going through the exercise it just didn't feel right to have some arguments non in the args structure just as much as it didn't feel right to have arguments to a call that would require ATTRIBUTE_UNUSED just so that I could have a common cb function argument list. I'll give one more shot at reworking things and then not worry about it.
As the patches progressively add more and more stuff into the callback data it really kills the advantage of using the callback at all. It's just separate functions hidden by a structure.
Extracting the loop into a function really doesn't make sense.
src/qemu/qemu_monitor_json.c | 345 ++++++++++++++++++++++++------------------- 1 file changed, 193 insertions(+), 152 deletions(-)
This also indicates that it didn't really help.
How much of this is additional function entry comments though... John
participants (2)
-
John Ferlan -
Peter Krempa