| Message ID | 1387482443-10633-5-git-send-email-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 12/19/2013 12:47 PM, Max Reitz wrote: > Reversing qdict_array_split(), qdict_flatten() should flatten QLists as > well by interpreting them as QDicts where every entry's key is its > index. > > This allows bringing QDicts with QLists from QMP commands to the same > form as they would be given as command-line options, thereby allowing > them to be parsed the same way. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > qobject/qdict.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++------ > 1 file changed, 51 insertions(+), 6 deletions(-) > Reviewed-by: Eric Blake <eblake@redhat.com>
On Thu, Dec 19, 2013 at 08:47:05PM +0100, Max Reitz wrote: > Reversing qdict_array_split(), qdict_flatten() should flatten QLists as > well by interpreting them as QDicts where every entry's key is its > index. > > This allows bringing QDicts with QLists from QMP commands to the same > form as they would be given as command-line options, thereby allowing > them to be parsed the same way. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > qobject/qdict.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++------ > 1 file changed, 51 insertions(+), 6 deletions(-) Please add a tests/check-qdict.c test case. > diff --git a/qobject/qdict.c b/qobject/qdict.c > index fca1902..7b6b08a 100644 > --- a/qobject/qdict.c > +++ b/qobject/qdict.c > @@ -477,7 +477,46 @@ static void qdict_destroy_obj(QObject *obj) > g_free(qdict); > } > > -static void qdict_do_flatten(QDict *qdict, QDict *target, const char *prefix) > +static void qdict_flatten_qdict(QDict *qdict, QDict *target, > + const char *prefix); > + > +static void qdict_flatten_qlist(QList *qlist, QDict *target, const char *prefix) > +{ > + QObject *value; > + const QListEntry *entry; > + char *new_key; > + int i; > + > + /* This function is never called with prefix == NULL, i.e., it is always > + * called from within qdict_flatten_q(list|dict)(). Therefore, it does not > + * need to remove list entries during the iteration (the whole list will be > + * deleted eventually anyway from qdict_flatten_qdict()). */ > + assert(prefix); > + > + entry = qlist_first(qlist); > + > + for (i = 0; entry; entry = qlist_next(entry), i++) { > + value = qlist_entry_obj(entry); > + > + qobject_incref(value); > + new_key = g_strdup_printf("%s.%i", prefix, i); > + qdict_put_obj(target, new_key, value); It seems this operation is clobbered by what follows and should be deleted. Is the incref also superfluous or... > + > + if (qobject_type(value) == QTYPE_QDICT) { > + qdict_flatten_qdict(qobject_to_qdict(value), target, new_key); > + } else if (qobject_type(value) == QTYPE_QLIST) { > + qdict_flatten_qlist(qobject_to_qlist(value), target, new_key); > + } else { > + /* All other types are moved to the target unchanged. */ > + qobject_incref(value); ...should this one be deleted? > + qdict_put_obj(target, new_key, value); > + } > + > + g_free(new_key); > + }
On 20.12.2013 10:46, Stefan Hajnoczi wrote: > On Thu, Dec 19, 2013 at 08:47:05PM +0100, Max Reitz wrote: >> Reversing qdict_array_split(), qdict_flatten() should flatten QLists as >> well by interpreting them as QDicts where every entry's key is its >> index. >> >> This allows bringing QDicts with QLists from QMP commands to the same >> form as they would be given as command-line options, thereby allowing >> them to be parsed the same way. >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> qobject/qdict.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++------ >> 1 file changed, 51 insertions(+), 6 deletions(-) > Please add a tests/check-qdict.c test case. > >> diff --git a/qobject/qdict.c b/qobject/qdict.c >> index fca1902..7b6b08a 100644 >> --- a/qobject/qdict.c >> +++ b/qobject/qdict.c >> @@ -477,7 +477,46 @@ static void qdict_destroy_obj(QObject *obj) >> g_free(qdict); >> } >> >> -static void qdict_do_flatten(QDict *qdict, QDict *target, const char *prefix) >> +static void qdict_flatten_qdict(QDict *qdict, QDict *target, >> + const char *prefix); >> + >> +static void qdict_flatten_qlist(QList *qlist, QDict *target, const char *prefix) >> +{ >> + QObject *value; >> + const QListEntry *entry; >> + char *new_key; >> + int i; >> + >> + /* This function is never called with prefix == NULL, i.e., it is always >> + * called from within qdict_flatten_q(list|dict)(). Therefore, it does not >> + * need to remove list entries during the iteration (the whole list will be >> + * deleted eventually anyway from qdict_flatten_qdict()). */ >> + assert(prefix); >> + >> + entry = qlist_first(qlist); >> + >> + for (i = 0; entry; entry = qlist_next(entry), i++) { >> + value = qlist_entry_obj(entry); >> + >> + qobject_incref(value); >> + new_key = g_strdup_printf("%s.%i", prefix, i); >> + qdict_put_obj(target, new_key, value); > It seems this operation is clobbered by what follows and should be > deleted. Is the incref also superfluous or... > >> + >> + if (qobject_type(value) == QTYPE_QDICT) { >> + qdict_flatten_qdict(qobject_to_qdict(value), target, new_key); >> + } else if (qobject_type(value) == QTYPE_QLIST) { >> + qdict_flatten_qlist(qobject_to_qlist(value), target, new_key); >> + } else { >> + /* All other types are moved to the target unchanged. */ >> + qobject_incref(value); > ...should this one be deleted? Oh great, I've been fixing working code, then. *g* I added the else branch in v5, I guess, it was correct not to have it (however, the condition in the else if was missing in v4). I'll drop it again, thanks. >> + qdict_put_obj(target, new_key, value); >> + } >> + >> + g_free(new_key); >> + }
On 20.12.2013 18:19, Max Reitz wrote: > On 20.12.2013 10:46, Stefan Hajnoczi wrote: >> On Thu, Dec 19, 2013 at 08:47:05PM +0100, Max Reitz wrote: >>> Reversing qdict_array_split(), qdict_flatten() should flatten QLists as >>> well by interpreting them as QDicts where every entry's key is its >>> index. >>> >>> This allows bringing QDicts with QLists from QMP commands to the same >>> form as they would be given as command-line options, thereby allowing >>> them to be parsed the same way. >>> >>> Signed-off-by: Max Reitz <mreitz@redhat.com> >>> --- >>> qobject/qdict.c | 57 >>> +++++++++++++++++++++++++++++++++++++++++++++++++++------ >>> 1 file changed, 51 insertions(+), 6 deletions(-) >> Please add a tests/check-qdict.c test case. >> >>> diff --git a/qobject/qdict.c b/qobject/qdict.c >>> index fca1902..7b6b08a 100644 >>> --- a/qobject/qdict.c >>> +++ b/qobject/qdict.c >>> @@ -477,7 +477,46 @@ static void qdict_destroy_obj(QObject *obj) >>> g_free(qdict); >>> } >>> -static void qdict_do_flatten(QDict *qdict, QDict *target, const >>> char *prefix) >>> +static void qdict_flatten_qdict(QDict *qdict, QDict *target, >>> + const char *prefix); >>> + >>> +static void qdict_flatten_qlist(QList *qlist, QDict *target, const >>> char *prefix) >>> +{ >>> + QObject *value; >>> + const QListEntry *entry; >>> + char *new_key; >>> + int i; >>> + >>> + /* This function is never called with prefix == NULL, i.e., it >>> is always >>> + * called from within qdict_flatten_q(list|dict)(). Therefore, >>> it does not >>> + * need to remove list entries during the iteration (the whole >>> list will be >>> + * deleted eventually anyway from qdict_flatten_qdict()). */ >>> + assert(prefix); >>> + >>> + entry = qlist_first(qlist); >>> + >>> + for (i = 0; entry; entry = qlist_next(entry), i++) { >>> + value = qlist_entry_obj(entry); >>> + >>> + qobject_incref(value); >>> + new_key = g_strdup_printf("%s.%i", prefix, i); >>> + qdict_put_obj(target, new_key, value); >> It seems this operation is clobbered by what follows and should be >> deleted. Is the incref also superfluous or... >> >>> + >>> + if (qobject_type(value) == QTYPE_QDICT) { >>> + qdict_flatten_qdict(qobject_to_qdict(value), target, >>> new_key); >>> + } else if (qobject_type(value) == QTYPE_QLIST) { >>> + qdict_flatten_qlist(qobject_to_qlist(value), target, >>> new_key); >>> + } else { >>> + /* All other types are moved to the target unchanged. */ >>> + qobject_incref(value); >> ...should this one be deleted? > > Oh great, I've been fixing working code, then. *g* > > I added the else branch in v5, I guess, it was correct not to have it > (however, the condition in the else if was missing in v4). I'll drop > it again, thanks. On second thought, that would be even more wrong. I'll leave the else branch and drop the qobject_incref() and qdict_put_obj() above the condition. Max > >>> + qdict_put_obj(target, new_key, value); >>> + } >>> + >>> + g_free(new_key); >>> + } >
On Fri, Dec 20, 2013 at 06:23:26PM +0100, Max Reitz wrote: > On 20.12.2013 18:19, Max Reitz wrote: > >On 20.12.2013 10:46, Stefan Hajnoczi wrote: > >>>diff --git a/qobject/qdict.c b/qobject/qdict.c > >>>index fca1902..7b6b08a 100644 > >>>--- a/qobject/qdict.c > >>>+++ b/qobject/qdict.c > >>>@@ -477,7 +477,46 @@ static void qdict_destroy_obj(QObject *obj) > >>> g_free(qdict); > >>> } > >>> -static void qdict_do_flatten(QDict *qdict, QDict *target, > >>>const char *prefix) > >>>+static void qdict_flatten_qdict(QDict *qdict, QDict *target, > >>>+ const char *prefix); > >>>+ > >>>+static void qdict_flatten_qlist(QList *qlist, QDict *target, > >>>const char *prefix) > >>>+{ > >>>+ QObject *value; > >>>+ const QListEntry *entry; > >>>+ char *new_key; > >>>+ int i; > >>>+ > >>>+ /* This function is never called with prefix == NULL, > >>>i.e., it is always > >>>+ * called from within qdict_flatten_q(list|dict)(). > >>>Therefore, it does not > >>>+ * need to remove list entries during the iteration (the > >>>whole list will be > >>>+ * deleted eventually anyway from qdict_flatten_qdict()). */ > >>>+ assert(prefix); > >>>+ > >>>+ entry = qlist_first(qlist); > >>>+ > >>>+ for (i = 0; entry; entry = qlist_next(entry), i++) { > >>>+ value = qlist_entry_obj(entry); > >>>+ > >>>+ qobject_incref(value); > >>>+ new_key = g_strdup_printf("%s.%i", prefix, i); > >>>+ qdict_put_obj(target, new_key, value); > >>It seems this operation is clobbered by what follows and should be > >>deleted. Is the incref also superfluous or... > >> > >>>+ > >>>+ if (qobject_type(value) == QTYPE_QDICT) { > >>>+ qdict_flatten_qdict(qobject_to_qdict(value), > >>>target, new_key); > >>>+ } else if (qobject_type(value) == QTYPE_QLIST) { > >>>+ qdict_flatten_qlist(qobject_to_qlist(value), > >>>target, new_key); > >>>+ } else { > >>>+ /* All other types are moved to the target unchanged. */ > >>>+ qobject_incref(value); > >>...should this one be deleted? > > > >Oh great, I've been fixing working code, then. *g* > > > >I added the else branch in v5, I guess, it was correct not to have > >it (however, the condition in the else if was missing in v4). I'll > >drop it again, thanks. > > On second thought, that would be even more wrong. I'll leave the > else branch and drop the qobject_incref() and qdict_put_obj() above > the condition. I agree, that sounds like the right solution.
diff --git a/qobject/qdict.c b/qobject/qdict.c index fca1902..7b6b08a 100644 --- a/qobject/qdict.c +++ b/qobject/qdict.c @@ -477,7 +477,46 @@ static void qdict_destroy_obj(QObject *obj) g_free(qdict); } -static void qdict_do_flatten(QDict *qdict, QDict *target, const char *prefix) +static void qdict_flatten_qdict(QDict *qdict, QDict *target, + const char *prefix); + +static void qdict_flatten_qlist(QList *qlist, QDict *target, const char *prefix) +{ + QObject *value; + const QListEntry *entry; + char *new_key; + int i; + + /* This function is never called with prefix == NULL, i.e., it is always + * called from within qdict_flatten_q(list|dict)(). Therefore, it does not + * need to remove list entries during the iteration (the whole list will be + * deleted eventually anyway from qdict_flatten_qdict()). */ + assert(prefix); + + entry = qlist_first(qlist); + + for (i = 0; entry; entry = qlist_next(entry), i++) { + value = qlist_entry_obj(entry); + + qobject_incref(value); + new_key = g_strdup_printf("%s.%i", prefix, i); + qdict_put_obj(target, new_key, value); + + if (qobject_type(value) == QTYPE_QDICT) { + qdict_flatten_qdict(qobject_to_qdict(value), target, new_key); + } else if (qobject_type(value) == QTYPE_QLIST) { + qdict_flatten_qlist(qobject_to_qlist(value), target, new_key); + } else { + /* All other types are moved to the target unchanged. */ + qobject_incref(value); + qdict_put_obj(target, new_key, value); + } + + g_free(new_key); + } +} + +static void qdict_flatten_qdict(QDict *qdict, QDict *target, const char *prefix) { QObject *value; const QDictEntry *entry, *next; @@ -500,8 +539,12 @@ static void qdict_do_flatten(QDict *qdict, QDict *target, const char *prefix) if (qobject_type(value) == QTYPE_QDICT) { /* Entries of QDicts are processed recursively, the QDict object * itself disappears. */ - qdict_do_flatten(qobject_to_qdict(value), target, - new_key ? new_key : entry->key); + qdict_flatten_qdict(qobject_to_qdict(value), target, + new_key ? new_key : entry->key); + delete = true; + } else if (qobject_type(value) == QTYPE_QLIST) { + qdict_flatten_qlist(qobject_to_qlist(value), target, + new_key ? new_key : entry->key); delete = true; } else if (prefix) { /* All other objects are moved to the target unchanged. */ @@ -526,12 +569,14 @@ static void qdict_do_flatten(QDict *qdict, QDict *target, const char *prefix) /** * 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". This operation - * is applied recursively for nested QDicts. + * 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. */ void qdict_flatten(QDict *qdict) { - qdict_do_flatten(qdict, qdict, NULL); + qdict_flatten_qdict(qdict, qdict, NULL); } /* extract all the src QDict entries starting by start into dst */
Reversing qdict_array_split(), qdict_flatten() should flatten QLists as well by interpreting them as QDicts where every entry's key is its index. This allows bringing QDicts with QLists from QMP commands to the same form as they would be given as command-line options, thereby allowing them to be parsed the same way. Signed-off-by: Max Reitz <mreitz@redhat.com> --- qobject/qdict.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 51 insertions(+), 6 deletions(-)