[Qemu-devel] [PATCH 0/3] Extract non-QDicts in qdict_array_split()

[Qemu-devel] [PATCH 0/3] Extract non-QDicts in qdict_array_split()

[Max Reitz]

Currently, qdict_array_split() splits a QDict like
  { "0.a": 42, "1": 23, "2.b": 84 }
into the QList
  [ { "a": 42 } ]
with the QDict still being
  { "1": 23, "2.b": 84 }

However, it makes more sense to create the QList
  [ { "a": 42 }, 23, { "b": 84 } ]
and having emptied the QDict.

This is implemented by this series.


Max Reitz (3):
  qemu-config: Sections must consist of keys
  qdict: Extract non-QDicts in qdict_array_split()
  check-qdict: Adjust test for qdict_array_split()

 qobject/qdict.c     | 60 ++++++++++++++++++++++++++++++++++++++++-------------
 tests/check-qdict.c | 22 ++++++++++++++------
 util/qemu-config.c  |  6 ++++++
 3 files changed, 68 insertions(+), 20 deletions(-)

-- 
1.9.0

[Qemu-devel] [PATCH 1/3] qemu-config: Sections must consist of keys

[Max Reitz]

In config_parse_qdict_section(), the QList returned by
qdict_array_split() is assumed to only contain QDicts. Currently, this
is true but it may (and will) change in the future. Therefore, check
whether the assumption actually holds.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 util/qemu-config.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/util/qemu-config.c b/util/qemu-config.c
index 797df71..f610101 100644
--- a/util/qemu-config.c
+++ b/util/qemu-config.c
@@ -413,6 +413,12 @@ static void config_parse_qdict_section(QDict *options, QemuOptsList *opts,
             QDict *section = qobject_to_qdict(qlist_entry_obj(list_entry));
             char *opt_name;
 
+            if (!section) {
+                error_setg(errp, "[%s] section (index %u) does not consist of "
+                           "keys", opts->name, i);
+                goto out;
+            }
+
             opt_name = g_strdup_printf("%s.%u", opts->name, i++);
             subopts = qemu_opts_create(opts, opt_name, 1, &local_err);
             g_free(opt_name);
-- 
1.9.0

Re: [Qemu-devel] [PATCH 1/3] qemu-config: Sections must consist of keys

[Eric Blake]

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

On 02/21/2014 11:11 AM, Max Reitz wrote:
> In config_parse_qdict_section(), the QList returned by
> qdict_array_split() is assumed to only contain QDicts. Currently, this
> is true but it may (and will) change in the future. Therefore, check
> whether the assumption actually holds.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  util/qemu-config.c | 6 ++++++
>  1 file changed, 6 insertions(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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 --]

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

[Max Reitz]

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(-)

diff --git a/qobject/qdict.c b/qobject/qdict.c
index a3924f2..42ec4c0 100644
--- a/qobject/qdict.c
+++ b/qobject/qdict.c
@@ -597,18 +597,33 @@ void qdict_extract_subqdict(QDict *src, QDict **dst, const char *start)
     }
 }
 
+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;
+        }
+    }
+
+    return false;
+}
+
 /**
  * qdict_array_split(): This function moves array-like elements of a QDict into
- * a new QList of QDicts. Every entry in the original QDict with a key prefixed
- * "%u.", where %u designates an unsigned integer starting at 0 and
+ * a new QList. Every entry in the original QDict with a key "%u" or one
+ * prefixed "%u.", where %u designates an unsigned integer starting at 0 and
  * incrementally counting up, will be moved to a new QDict at index %u in the
- * output QList with the key prefix removed. The function terminates when there
- * is no entry in the QDict with a prefix directly (incrementally) following the
- * last one.
- * Example: {"0.a": 42, "0.b": 23, "1.x": 0, "3.y": 1, "o.o": 7}
- *      (or {"1.x": 0, "3.y": 1, "0.a": 42, "o.o": 7, "0.b": 23})
- *       => [{"a": 42, "b": 23}, {"x": 0}]
- *      and {"3.y": 1, "o.o": 7} (remainder of the old QDict)
+ * output QList with the key prefix removed, if that prefix is "%u.". If the
+ * whole key is just "%u", the whole QObject will be moved unchanged without
+ * creating a new QDict. The function terminates when there is no entry in the
+ * QDict with a prefix directly (incrementally) following the last one; it also
+ * returns if there are both entries with "%u" and "%u." for the same index %u.
+ * Example: {"0.a": 42, "0.b": 23, "1.x": 0, "4.y": 1, "o.o": 7, "2": 66}
+ *      (or {"1.x": 0, "4.y": 1, "0.a": 42, "o.o": 7, "0.b": 23, "2": 66})
+ *       => [{"a": 42, "b": 23}, {"x": 0}, 66]
+ *      and {"4.y": 1, "o.o": 7} (remainder of the old QDict)
  */
 void qdict_array_split(QDict *src, QList **dst)
 {
@@ -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);
+
+        subqobj = qdict_get(src, indexstr);
+
         snprintf_ret = snprintf(prefix, 32, "%u.", i);
         assert(snprintf_ret < 32);
 
-        qdict_extract_subqdict(src, &subqdict, prefix);
-        if (!qdict_size(subqdict)) {
-            QDECREF(subqdict);
+        is_subqdict = qdict_has_prefixed_entries(src, prefix);
+
+        // There may be either a single subordinate object (named "%u") or
+        // multiple objects (each with a key prefixed "%u."), but not both.
+        if (!subqobj == !is_subqdict) {
             break;
         }
 
-        qlist_append_obj(*dst, QOBJECT(subqdict));
+        if (is_subqdict) {
+            qdict_extract_subqdict(src, &subqdict, prefix);
+            assert(qdict_size(subqdict) > 0);
+        } else {
+            qobject_incref(subqobj);
+            qdict_del(src, indexstr);
+        }
+
+        qlist_append_obj(*dst, subqobj ?: QOBJECT(subqdict));
     }
 }
-- 
1.9.0

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

[Eric Blake]

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

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...

> +
> +        subqobj = qdict_get(src, indexstr);
> +
>          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>

-- 
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 --]

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

[Max Reitz]

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>
>

[Qemu-devel] [PATCH 3/3] check-qdict: Adjust test for qdict_array_split()

[Max Reitz]

Test the new functionality of qdict_array_split(), that is, splitting
off single objects.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/check-qdict.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/tests/check-qdict.c b/tests/check-qdict.c
index 7a7461b..680e3c3 100644
--- a/tests/check-qdict.c
+++ b/tests/check-qdict.c
@@ -306,6 +306,7 @@ static void qdict_array_split_test(void)
 {
     QDict *test_dict = qdict_new();
     QDict *dict1, *dict2;
+    QInt *int1;
     QList *test_list;
 
     /*
@@ -313,10 +314,11 @@ static void qdict_array_split_test(void)
      *
      * {
      *     "1.x": 0,
-     *     "3.y": 1,
+     *     "4.y": 1,
      *     "0.a": 42,
      *     "o.o": 7,
-     *     "0.b": 23
+     *     "0.b": 23,
+     *     "2": 66
      * }
      *
      * to
@@ -328,13 +330,14 @@ static void qdict_array_split_test(void)
      *     },
      *     {
      *         "x": 0
-     *     }
+     *     },
+     *     66
      * ]
      *
      * and
      *
      * {
-     *     "3.y": 1,
+     *     "4.y": 1,
      *     "o.o": 7
      * }
      *
@@ -344,18 +347,21 @@ static void qdict_array_split_test(void)
      */
 
     qdict_put(test_dict, "1.x", qint_from_int(0));
-    qdict_put(test_dict, "3.y", qint_from_int(1));
+    qdict_put(test_dict, "4.y", qint_from_int(1));
     qdict_put(test_dict, "0.a", qint_from_int(42));
     qdict_put(test_dict, "o.o", qint_from_int(7));
     qdict_put(test_dict, "0.b", qint_from_int(23));
+    qdict_put(test_dict, "2", qint_from_int(66));
 
     qdict_array_split(test_dict, &test_list);
 
     dict1 = qobject_to_qdict(qlist_pop(test_list));
     dict2 = qobject_to_qdict(qlist_pop(test_list));
+    int1 = qobject_to_qint(qlist_pop(test_list));
 
     g_assert(dict1);
     g_assert(dict2);
+    g_assert(int1);
     g_assert(qlist_empty(test_list));
 
     QDECREF(test_list);
@@ -373,7 +379,11 @@ static void qdict_array_split_test(void)
 
     QDECREF(dict2);
 
-    g_assert(qdict_get_int(test_dict, "3.y") == 1);
+    g_assert(qint_get_int(int1) == 66);
+
+    QDECREF(int1);
+
+    g_assert(qdict_get_int(test_dict, "4.y") == 1);
     g_assert(qdict_get_int(test_dict, "o.o") == 7);
 
     g_assert(qdict_size(test_dict) == 2);
-- 
1.9.0

Re: [Qemu-devel] [PATCH 3/3] check-qdict: Adjust test for qdict_array_split()

[Eric Blake]

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

On 02/21/2014 11:11 AM, Max Reitz wrote:
> Test the new functionality of qdict_array_split(), that is, splitting
> off single objects.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/check-qdict.c | 22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

However, I think it is incomplete - you didn't test the behavior when
both %u and %u.sub appear in the same dict, to make sure the splitting
stops at that point.

-- 
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 --]

Re: [Qemu-devel] [PATCH 3/3] check-qdict: Adjust test for qdict_array_split()

[Max Reitz]

On 21.02.2014 19:39, Eric Blake wrote:
> On 02/21/2014 11:11 AM, Max Reitz wrote:
>> Test the new functionality of qdict_array_split(), that is, splitting
>> off single objects.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   tests/check-qdict.c | 22 ++++++++++++++++------
>>   1 file changed, 16 insertions(+), 6 deletions(-)
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
> However, I think it is incomplete - you didn't test the behavior when
> both %u and %u.sub appear in the same dict, to make sure the splitting
> stops at that point.

Yes, I'll add a test for that case, too.


Thanks for you review,

Max

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

[Kevin Wolf]

Am 21.02.2014 um 19:11 hat Max Reitz geschrieben:
> Currently, qdict_array_split() splits a QDict like
>   { "0.a": 42, "1": 23, "2.b": 84 }
> into the QList
>   [ { "a": 42 } ]
> with the QDict still being
>   { "1": 23, "2.b": 84 }
> 
> However, it makes more sense to create the QList
>   [ { "a": 42 }, 23, { "b": 84 } ]
> and having emptied the QDict.
> 
> This is implemented by this series.

Thanks, applied to the block branch.

Kevin

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

[Eric Blake]

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

On 02/21/2014 11:11 AM, Max Reitz wrote:
> Currently, qdict_array_split() splits a QDict like
>   { "0.a": 42, "1": 23, "2.b": 84 }
> into the QList
>   [ { "a": 42 } ]
> with the QDict still being
>   { "1": 23, "2.b": 84 }
> 
> However, it makes more sense to create the QList
>   [ { "a": 42 }, 23, { "b": 84 } ]
> and having emptied the QDict.
> 
> This is implemented by this series.
> 

Question - in the code, we have a comment:

/**
 * qdict_flatten(): For each nested QDict with key x, all fields with key y
 * are moved to this QDict and their key is renamed to "x.y". For each
nested
 * QList with key x, the field at index y is moved to this QDict with
the key
 * "x.y" (i.e., the reverse of what qdict_array_split() does).
 * This operation is applied recursively for nested QDicts and QLists.
 */

With your new split rules, do we need a followup patch to qdict_flatten
that can regenerate the QDict with "%u" keys for non-dict members of the
QList?

-- 
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 --]

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

[Max Reitz]

On 21.02.2014 23:32, Eric Blake wrote:
> On 02/21/2014 11:11 AM, Max Reitz wrote:
>> Currently, qdict_array_split() splits a QDict like
>>    { "0.a": 42, "1": 23, "2.b": 84 }
>> into the QList
>>    [ { "a": 42 } ]
>> with the QDict still being
>>    { "1": 23, "2.b": 84 }
>>
>> However, it makes more sense to create the QList
>>    [ { "a": 42 }, 23, { "b": 84 } ]
>> and having emptied the QDict.
>>
>> This is implemented by this series.
>>
> Question - in the code, we have a comment:
>
> /**
>   * qdict_flatten(): For each nested QDict with key x, all fields with key y
>   * are moved to this QDict and their key is renamed to "x.y". For each
> nested
>   * QList with key x, the field at index y is moved to this QDict with
> the key
>   * "x.y" (i.e., the reverse of what qdict_array_split() does).
>   * This operation is applied recursively for nested QDicts and QLists.
>   */
>
> With your new split rules, do we need a followup patch to qdict_flatten
> that can regenerate the QDict with "%u" keys for non-dict members of the
> QList?

If you want qdict_flatten() to return a QDict with "%u" keys, you'd have 
to specify a QList. However, this would no longer be a qdict_flatten(), 
but rather a qlist_flatten().

It would be easy to make use of qdict_flatten_qdict() and 
qdict_flatten_qlist() to implement qlist_flatten() as well, but since 
there is currently no such function, apparently there is no need for it. 
Of course, I could just implement it preemptively, but then I'd probably 
be asked for its purpose. ;-)


Max

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

[Eric Blake]

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

On 02/26/2014 10:16 AM, Max Reitz wrote:
>>   * "x.y" (i.e., the reverse of what qdict_array_split() does).
>>   * This operation is applied recursively for nested QDicts and QLists.
>>   */
>>
>> With your new split rules, do we need a followup patch to qdict_flatten
>> that can regenerate the QDict with "%u" keys for non-dict members of the
>> QList?
> 
> If you want qdict_flatten() to return a QDict with "%u" keys, you'd have
> to specify a QList. However, this would no longer be a qdict_flatten(),
> but rather a qlist_flatten().

Fair enough - nothing further needed at this time.  :)

-- 
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 --]