Message ID | 1373363617-4723-10-git-send-email-kwolf@redhat.com |
---|---|
State | New |
Headers | show |
On 07/09/2013 03:53 AM, Kevin Wolf wrote: Worth repeating this comment from the code into the commit message? > + * 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". Otherwise, I had to read nearly the entire patch to learn what I was supposed to be reviewing. > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > include/qapi/qmp/qdict.h | 1 + > qobject/qdict.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 48 insertions(+) > > +++ b/qobject/qdict.c > @@ -476,3 +476,50 @@ static void qdict_destroy_obj(QObject *obj) > > g_free(qdict); > } > + > +static void qdict_do_flatten(QDict *qdict, QDict *target, const char *prefix) > +{ > + QObject *value; > + const QDictEntry *entry, *next; > + const char *new_key; > + bool delete; > + > + entry = qdict_first(qdict); > + > + while (entry != NULL) { > + > + next = qdict_next(qdict, entry); > + value = qdict_entry_value(entry); > + new_key = NULL; > + delete = false; > + > + if (prefix) { > + qobject_incref(value); > + new_key = g_strdup_printf("%s.%s", prefix, entry->key); > + qdict_put_obj(target, new_key, value); You are calling this function with the same parameter for both qdict and target. Doesn't that mean you are modifying qdict while iterating it? Is that safe? [/me re-reads] - oh, you recursively call this function, and this modification of target happens _only_ if prefix is non-null, which happens only: > + delete = true; > + } > + > + if (qobject_type(value) == QTYPE_QDICT) { > + qdict_do_flatten(qobject_to_qdict(value), target, > + new_key ? new_key : entry->key); when passing two different dicts into the function. Maybe add an assert(!prefix || qdict != target). > + delete = true; > + } > + > + if (delete) { > + qdict_del(qdict, entry->key); > + } > + > + entry = next; > + } > +} > + > +/** > + * 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". > + */ > +void qdict_flatten(QObject *obj) > +{ > + QDict *qdict = qobject_to_qdict(obj); > + qdict_do_flatten(qdict, qdict, NULL); > +} >
Am 11.07.2013 um 22:25 hat Eric Blake geschrieben: > On 07/09/2013 03:53 AM, Kevin Wolf wrote: > > Worth repeating this comment from the code into the commit message? > > > + * 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". > > Otherwise, I had to read nearly the entire patch to learn what I was > supposed to be reviewing. Okay, will do that. > > +static void qdict_do_flatten(QDict *qdict, QDict *target, const char *prefix) > > +{ > > + QObject *value; > > + const QDictEntry *entry, *next; > > + const char *new_key; > > + bool delete; > > + > > + entry = qdict_first(qdict); > > + > > + while (entry != NULL) { > > + > > + next = qdict_next(qdict, entry); > > + value = qdict_entry_value(entry); > > + new_key = NULL; > > + delete = false; > > + > > + if (prefix) { > > + qobject_incref(value); > > + new_key = g_strdup_printf("%s.%s", prefix, entry->key); > > + qdict_put_obj(target, new_key, value); > > You are calling this function with the same parameter for both qdict and > target. Doesn't that mean you are modifying qdict while iterating it? > Is that safe? [/me re-reads] - oh, you recursively call this function, > and this modification of target happens _only_ if prefix is non-null, > which happens only: > > > + delete = true; > > + } > > + > > + if (qobject_type(value) == QTYPE_QDICT) { > > + qdict_do_flatten(qobject_to_qdict(value), target, > > + new_key ? new_key : entry->key); > > when passing two different dicts into the function. Maybe add an > assert(!prefix || qdict != target). Your point still stands: The recursively called function modifies target (which is qdict on the top level) by adding new keys. I guess when it returns I need to restart the loop from the beginning in order to be safe. Kevin > > + delete = true; > > + } > > + > > + if (delete) { > > + qdict_del(qdict, entry->key); > > + } > > + > > + entry = next; > > + } > > +} > > + > > +/** > > + * 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". > > + */ > > +void qdict_flatten(QObject *obj) > > +{ > > + QDict *qdict = qobject_to_qdict(obj); > > + qdict_do_flatten(qdict, qdict, NULL); > > +} > > > > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org >
diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h index 685b2e3..b261570 100644 --- a/include/qapi/qmp/qdict.h +++ b/include/qapi/qmp/qdict.h @@ -65,5 +65,6 @@ int qdict_get_try_bool(const QDict *qdict, const char *key, int def_value); const char *qdict_get_try_str(const QDict *qdict, const char *key); QDict *qdict_clone_shallow(const QDict *src); +void qdict_flatten(QObject *obj); #endif /* QDICT_H */ diff --git a/qobject/qdict.c b/qobject/qdict.c index ed381f9..1c38943 100644 --- a/qobject/qdict.c +++ b/qobject/qdict.c @@ -476,3 +476,50 @@ static void qdict_destroy_obj(QObject *obj) g_free(qdict); } + +static void qdict_do_flatten(QDict *qdict, QDict *target, const char *prefix) +{ + QObject *value; + const QDictEntry *entry, *next; + const char *new_key; + bool delete; + + entry = qdict_first(qdict); + + while (entry != NULL) { + + next = qdict_next(qdict, entry); + value = qdict_entry_value(entry); + new_key = NULL; + delete = false; + + if (prefix) { + qobject_incref(value); + new_key = g_strdup_printf("%s.%s", prefix, entry->key); + qdict_put_obj(target, new_key, value); + delete = true; + } + + if (qobject_type(value) == QTYPE_QDICT) { + qdict_do_flatten(qobject_to_qdict(value), target, + new_key ? new_key : entry->key); + delete = true; + } + + if (delete) { + qdict_del(qdict, entry->key); + } + + entry = next; + } +} + +/** + * 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". + */ +void qdict_flatten(QObject *obj) +{ + QDict *qdict = qobject_to_qdict(obj); + qdict_do_flatten(qdict, qdict, NULL); +}
Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/qapi/qmp/qdict.h | 1 + qobject/qdict.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+)