Re: [libvirt] [PATCH 01/15] util: json: add helper to iterate and steal members of json array
On 10/20/2016 10:24 AM, Peter Krempa wrote:
Simplifies cases where JSON array members need to be transferred to
a
different structure.
---
src/libvirt_private.syms | 1 +
src/util/virjson.c | 36 ++++++++++++++++++++++++++++++++++++
src/util/virjson.h | 6 ++++++
3 files changed, 43 insertions(+)
+/**
+ * virJSONValueArrayForeachSteal:
+ * @array: array to iterate
+ * @cb: callback called on every member of the array
+ * @opaque: custom data for the callback
+ *
+ * Iterates members of the array and calls the callback on every single member.
+ * By returning success, the array claims ownership of given array element and
s/array claims/callback claims/
"returning success" - is that 0?
+ * is responsible for freeing it.
+ *
+ * Returns 0 if all members were successfully stolen by the callback; -1
+ * otherwise. The rest of the members stays in posession of the array.
s/stays/stay/
s/posession/the possession/
Does the iteration stop at the first callback that returns non-zero? Do
we want something a bit more complex, where a negative return aborts
right away, returning 0 means the callback claimed possession but keep
iterating, and where a positive return means the callback did not claim
possession but wants to keep iterating?
+ */
+int
+virJSONValueArrayForeachSteal(virJSONValuePtr array,
+ virJSONArrayIteratorFunc cb,
+ void *opaque)
+{
+ ssize_t i;
+
+ if (array->type != VIR_JSON_TYPE_ARRAY)
+ return -1;
+
+ for (i = array->data.array.nvalues - 1; i >= 0; i--) {
Does order of iteration matter? JSON arrays (can be) order-dependent,
unlike dicts that are order-agnostic. All other clients of
data.array.nvalues iterate forward, not backward, so we ought to do
likewise.
+ if (cb(i, array->data.array.values[i], opaque) < 0)
+ break;
Okay, you are handling < 0 as failure and end iteration, and treating
all non-negative as successfully claiming the element. But is that
enough power for all uses of this interface? See my thoughts above
about distinguishing between 0 and positive returns. Then again, if you
do that, you'd have to memmove remaining elements to close gaps.
+
+ array->data.array.values[i] = NULL;
+ }
+
+ array->data.array.nvalues = i + 1;
I see why you iterate backwards; you don't have to memmove elements that
aren't stolen. But I'm still not convinced it is the best thing to be
doing.
+
+ return i < 0 ? 0 : -1;
+}
The interface sounds like it will be useful, but I want to make sure it
doesn't lock us into design issues that will haunt us later on.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 604 bytes)