1:39 p.m.
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
1:39 p.m.
New subject: [libvirt] [PATCH v2 1/5] qemu: Create common code for JSON "query-block" call
Reduce some cut-n-paste code by creating common helper
NB: This also adds error checking to qemuMonitorJSONDiskNameLookup
Signed-off-by: John Ferlan <jferlan(a)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
1:39 p.m.
New subject: [libvirt] [PATCH v2 2/5] qemu: Split out filling of JSONGetBlockInfo data
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(a)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
1:39 p.m.
New subject: [libvirt] [PATCH v2 3/5] qemu: Split out filling of JSONBlockStats data
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(a)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
1:39 p.m.
New subject: [libvirt] [PATCH v2 4/5] qemu: Split out filling of JSONDiskNameLookup data
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(a)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
1:39 p.m.
New subject: [libvirt] [PATCH v2 5/5] qemu: Combine the various ways to call query-block
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(a)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
2:22 a.m.
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
6:49 a.m.
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
7:56 a.m.
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
9:46 a.m.
[...]
> 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
9:49 a.m.
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.
2:21 p.m.
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
3007
days inactive
3010
days old
11 comments
2 participants
participants (2)
-
John Ferlan -
Peter Krempa