[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH 01/34] qdict: Add qdict_array_entri
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH 01/34] qdict: Add qdict_array_entries() |
Date: |
Mon, 11 May 2015 16:40:44 +0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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 <address@hidden>
> > ---
>
> 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
pgpaQRVRUM92m.pgp
Description: PGP signature