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
>
