Re: [Qemu-block] [Qemu-devel] [PATCH 01/34] qdict: Add qdict_array_entries()
On Fri 08 May 2015 10:06:35 PM CEST, Eric Blake wrote:
+for (i = 0; i UINT_MAX; i++) {
+QObject *subqobj;
+int subqdict_entries;
+size_t slen = 32 + subqdict_len;
+char indexstr[slen], prefix[slen];
And more dependence on a working C99 compiler, thanks to variable
length array (VLA).
+size_t snprintf_ret;
+
+snprintf_ret = snprintf(indexstr, slen, %s%u, subqdict, i);
+assert(snprintf_ret slen);
Since gcc may compile the allocation of indexstr into a malloc()
anyways, would it be any simpler to just use g_strdup_printf()
directly, instead of futzing around with VLA and snprintf() ourselves?
It might mean less code, as some of the error checking is taken care
of on your behalf.
Since the only difference between the two strings you are allocating is
the trailing dot, you could also save one malloc() by reusing the same
string and stripping the dot.
Alternatively you could allocate the memory outside the loop instead of
having to do it in every iteration. The size is always the same after
all.
Berto
Re: [Qemu-block] [Qemu-devel] [PATCH 01/34] qdict: Add qdict_array_entries()
Am 08.05.2015 um 22:06 hat Eric Blake geschrieben:
On 05/08/2015 11:21 AM, Kevin Wolf wrote:
Signed-off-by: Kevin Wolf kw...@redhat.com
---
Might want to include mention of what it will be used for in the commit
body.
You're right. This is the new commit message:
This counts the entries in a flattened array in a QDict without
actually splitting the QDict into a QList.
bdrv_open_image() doesn't take a QList, but rather a QDict and a key
prefix string, so this is more convenient for block drivers which have a
dynamically sized list of child nodes (e.g. Quorum) and are to be
converted to using bdrv_open_image() as the standard interface for
opening child nodes.
include/qapi/qmp/qdict.h | 1 +
qobject/qdict.c | 68
+---
2 files changed, 65 insertions(+), 4 deletions(-)
@@ -646,7 +647,7 @@ void qdict_array_split(QDict *src, QList **dst)
snprintf_ret = snprintf(prefix, 32, %u., i);
assert(snprintf_ret 32);
-is_subqdict = qdict_has_prefixed_entries(src, prefix);
+is_subqdict = qdict_count_prefixed_entries(src, prefix);
Good thing we require a C99 compiler, where 'bool = int' does the right
thing for integers 1.
/**
+ * qdict_array_valid(): Returns the number of direct array entries if the
+ * sub-QDict of src specified by the prefix in subqdict (or src itself for
+ * prefix == ) is valid as an array, i.e. the length of the created list
if
+ * the sub-QDict would become empty after calling qdict_array_split() on
it. If
+ * the array is not valid, -1 is returned.
+ */
+int qdict_array_entries(QDict *src, const char *subqdict)
Comment doesn't match function name.
Thanks, fixed.
+{
+const QDictEntry *entry;
+unsigned i;
+unsigned entries = 0;
+size_t subqdict_len = strlen(subqdict);
+
+assert(!subqdict_len || subqdict[subqdict_len - 1] == '.');
+
+for (i = 0; i UINT_MAX; i++) {
+QObject *subqobj;
+int subqdict_entries;
+size_t slen = 32 + subqdict_len;
+char indexstr[slen], prefix[slen];
And more dependence on a working C99 compiler, thanks to variable length
array (VLA).
+size_t snprintf_ret;
+
+snprintf_ret = snprintf(indexstr, slen, %s%u, subqdict, i);
+assert(snprintf_ret slen);
Since gcc may compile the allocation of indexstr into a malloc()
anyways, would it be any simpler to just use g_strdup_printf() directly,
instead of futzing around with VLA and snprintf() ourselves? It might
mean less code, as some of the error checking is taken care of on your
behalf.
This code parallels the code in qdict_array_split(), which looks almos
the same, except that the latter doesn't support subqict and therefore
has a fixed-size array rather than a VLA.
If you think that g_strdup_printf() is preferable, I would do that on
top of this series and change both functions.
Kevin
pgp3qwW3kJDaB.pgp
Description: PGP signature
Re: [Qemu-block] [Qemu-devel] [PATCH 01/34] qdict: Add qdict_array_entries()
On 05/11/2015 08:40 AM, Kevin Wolf wrote: +char indexstr[slen], prefix[slen]; And more dependence on a working C99 compiler, thanks to variable length array (VLA). +size_t snprintf_ret; + +snprintf_ret = snprintf(indexstr, slen, %s%u, subqdict, i); +assert(snprintf_ret slen); Since gcc may compile the allocation of indexstr into a malloc() anyways, would it be any simpler to just use g_strdup_printf() directly, instead of futzing around with VLA and snprintf() ourselves? It might mean less code, as some of the error checking is taken care of on your behalf. This code parallels the code in qdict_array_split(), which looks almos the same, except that the latter doesn't support subqict and therefore has a fixed-size array rather than a VLA. If you think that g_strdup_printf() is preferable, I would do that on top of this series and change both functions. I'm not strongly opposed to keeping snprintf, but agree that if you want to clean it up to g_strdup_printf(), a separate patch hitting multiple uses would be cleaner than respinning this patch. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [Qemu-devel] [PATCH 01/34] qdict: Add qdict_array_entries()
On 05/08/2015 11:21 AM, Kevin Wolf wrote:
Signed-off-by: Kevin Wolf kw...@redhat.com
---
Might want to include mention of what it will be used for in the commit
body.
include/qapi/qmp/qdict.h | 1 +
qobject/qdict.c | 68
+---
2 files changed, 65 insertions(+), 4 deletions(-)
@@ -646,7 +647,7 @@ void qdict_array_split(QDict *src, QList **dst)
snprintf_ret = snprintf(prefix, 32, %u., i);
assert(snprintf_ret 32);
-is_subqdict = qdict_has_prefixed_entries(src, prefix);
+is_subqdict = qdict_count_prefixed_entries(src, prefix);
Good thing we require a C99 compiler, where 'bool = int' does the right
thing for integers 1.
/**
+ * qdict_array_valid(): Returns the number of direct array entries if the
+ * sub-QDict of src specified by the prefix in subqdict (or src itself for
+ * prefix == ) is valid as an array, i.e. the length of the created list if
+ * the sub-QDict would become empty after calling qdict_array_split() on it.
If
+ * the array is not valid, -1 is returned.
+ */
+int qdict_array_entries(QDict *src, const char *subqdict)
Comment doesn't match function name.
+{
+const QDictEntry *entry;
+unsigned i;
+unsigned entries = 0;
+size_t subqdict_len = strlen(subqdict);
+
+assert(!subqdict_len || subqdict[subqdict_len - 1] == '.');
+
+for (i = 0; i UINT_MAX; i++) {
+QObject *subqobj;
+int subqdict_entries;
+size_t slen = 32 + subqdict_len;
+char indexstr[slen], prefix[slen];
And more dependence on a working C99 compiler, thanks to variable length
array (VLA).
+size_t snprintf_ret;
+
+snprintf_ret = snprintf(indexstr, slen, %s%u, subqdict, i);
+assert(snprintf_ret slen);
Since gcc may compile the allocation of indexstr into a malloc()
anyways, would it be any simpler to just use g_strdup_printf() directly,
instead of futzing around with VLA and snprintf() ourselves? It might
mean less code, as some of the error checking is taken care of on your
behalf.
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
