From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57742) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Uz16H-0000zE-Jg for qemu-devel@nongnu.org; Tue, 16 Jul 2013 04:59:14 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Uz16G-0005ex-FR for qemu-devel@nongnu.org; Tue, 16 Jul 2013 04:59:13 -0400 Received: from mx1.redhat.com ([209.132.183.28]:12859) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Uz16G-0005er-4v for qemu-devel@nongnu.org; Tue, 16 Jul 2013 04:59:12 -0400 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r6G8xA5m031030 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 16 Jul 2013 04:59:11 -0400 Date: Tue, 16 Jul 2013 10:59:08 +0200 From: Kevin Wolf Message-ID: <20130716085908.GD2387@dhcp-200-207.str.redhat.com> References: <1373363617-4723-1-git-send-email-kwolf@redhat.com> <1373363617-4723-10-git-send-email-kwolf@redhat.com> <51DF14CC.9000609@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <51DF14CC.9000609@redhat.com> Subject: Re: [Qemu-devel] [RFC PATCH 09/11] Implement qdict_flatten() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: armbru@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, lcapitulino@redhat.com 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 >