Re: [Qemu-devel] [PATCH 2/3] qdict: Extract non-QDicts in qdict_array_split()

[Max Reitz <mreitz@redhat.com>]

On 21.02.2014 19:37, Eric Blake wrote:
> On 02/21/2014 11:11 AM, Max Reitz wrote:
>> Currently, qdict_array_split() only splits off entries with a key prefix
>> of "%u.", packing them into a new QDict. This patch makes it support
>> entries with the plain key "%u" as well, directly putting them into the
>> new QList without creating a QDict.
>>
>> If there is both an entry with a key of "%u" and other entries with keys
>> prefixed "%u." (for the same index), the function simply terminates.
>>
>> To do this, this patch also adds a static function which tests whether a
>> given QDict contains any keys with the given prefix. This is used to test
>> whether entries with a key prefixed "%u." do exist in the source QDict
>> without modifying it.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   qobject/qdict.c | 60 +++++++++++++++++++++++++++++++++++++++++++--------------
>>   1 file changed, 46 insertions(+), 14 deletions(-)
>>
>> +static bool qdict_has_prefixed_entries(const QDict *src, const char *start)
>> +{
>> +    const QDictEntry *entry;
>> +
>> +    for (entry = qdict_first(src); entry; entry = qdict_next(src, entry)) {
>> +        if (strstart(entry->key, start, NULL)) {
>> +            return true;
> Note that if called with start="1" and the dict contains a key "10",
> this would return true.
>
>> @@ -617,19 +632,36 @@ void qdict_array_split(QDict *src, QList **dst)
>>       *dst = qlist_new();
>>   
>>       for (i = 0; i < UINT_MAX; i++) {
>> +        QObject *subqobj;
>> +        bool is_subqdict;
>>           QDict *subqdict;
>> -        char prefix[32];
>> +        char indexstr[32], prefix[32];
>>           size_t snprintf_ret;
>>   
>> +        snprintf_ret = snprintf(indexstr, 32, "%u", i);
>> +        assert(snprintf_ret < 32);
> This assertion is redundant...

Not really, as...

>> +
>> +        subqobj = qdict_get(src, indexstr);
>> +

...this qdict_get() may break if we haven't asserted that indexstr is 
indeed 0-terminated at this point (e.g. by testing that snprintf() 
didn't use the whole space, as I did).

Max

>>           snprintf_ret = snprintf(prefix, 32, "%u.", i);
>>           assert(snprintf_ret < 32);
> ...if this assertion about a longer string holds true.  But it doesn't
> hurt my feelings to leave it in.
>
>>   
>> -        qdict_extract_subqdict(src, &subqdict, prefix);
>> -        if (!qdict_size(subqdict)) {
>> -            QDECREF(subqdict);
>> +        is_subqdict = qdict_has_prefixed_entries(src, prefix);
> Thankfully you always test a prefix with a trailing '.', so this is not
> a problem in your usage.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>