Re: [Qemu-devel] [PATCH 01/34] qdict: Add qdict_array_entries()

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2326 bytes --]

On 05/08/2015 11:21 AM, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]