From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55147) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VrV1k-0004Tw-EF for qemu-devel@nongnu.org; Fri, 13 Dec 2013 10:51:58 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VrV1X-0006vr-7O for qemu-devel@nongnu.org; Fri, 13 Dec 2013 10:51:44 -0500 Received: from mx1.redhat.com ([209.132.183.28]:11781) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VrV1W-0006uc-VB for qemu-devel@nongnu.org; Fri, 13 Dec 2013 10:51:31 -0500 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id rBDFpSQX022138 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Fri, 13 Dec 2013 10:51:29 -0500 Message-ID: <52AB2D36.8080701@redhat.com> Date: Fri, 13 Dec 2013 16:52:22 +0100 From: Max Reitz MIME-Version: 1.0 References: <1386785473-26157-1-git-send-email-mreitz@redhat.com> <1386785473-26157-5-git-send-email-mreitz@redhat.com> <52A95517.3020303@redhat.com> In-Reply-To: <52A95517.3020303@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v3 04/21] qapi: extend qdict_flatten() for QLists List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng , qemu-devel@nongnu.org Cc: Kevin Wolf , Stefan Hajnoczi On 12.12.2013 07:17, Fam Zheng wrote: > On 2013=E5=B9=B412=E6=9C=8812=E6=97=A5 02:10, Max Reitz wrote: >> Reversing qdict_array_split(), qdict_flatten() should flatten QLists a= s >> 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 >> --- > > The logic looks fine, just a few questions about assertions below. > >> qobject/qdict.c | 45 +++++++++++++++++++++++++++++++++++++++++---- >> 1 file changed, 41 insertions(+), 4 deletions(-) >> >> diff --git a/qobject/qdict.c b/qobject/qdict.c >> index fca1902..d7ce4b3 100644 >> --- a/qobject/qdict.c >> +++ b/qobject/qdict.c >> @@ -477,7 +477,40 @@ static void qdict_destroy_obj(QObject *obj) >> g_free(qdict); >> } >> >> -static void qdict_do_flatten(QDict *qdict, QDict *target, const char=20 >> *prefix) >> +static void qdict_flatten_qdict(QDict *qdict, QDict *target, >> + const char *prefix); >> + >> +static void qdict_flatten_qlist(QList *qlist, QDict *target, const=20 >> char *prefix) >> +{ >> + QObject *value; >> + const QListEntry *entry; >> + char *new_key; >> + int i; >> + >> + /* This function is never called with prefix =3D=3D NULL, i.e., it i= s=20 >> always >> + * called from within qdict_flatten_q(list|dict)(). Therefore, it=20 >> does not >> + * need to remove list entries during the iteration (the whole list=20 >> will be >> + * deleted eventually anyway from qdict_flatten_qdict()). Also,=20 >> prefix can >> + * never be NULL. */ > > Then maybe assert this? Yes, I'll do that. > >> + >> + entry =3D qlist_first(qlist); >> + >> + for (i =3D 0; entry; entry =3D qlist_next(entry), i++) { >> + value =3D qlist_entry_obj(entry); >> + >> + qobject_incref(value); >> + new_key =3D g_strdup_printf("%s.%i", prefix, i); >> + qdict_put_obj(target, new_key, value); >> + >> + if (qobject_type(value) =3D=3D QTYPE_QDICT) { >> + qdict_flatten_qdict(qobject_to_qdict(value), target, new_key); >> + } else { > > And maybe assert the type are not something other than dict and list? I think I rather forgot to support other types. Those should be moved to=20 the target QDict, just as qdict_flatten_qdict() does it. > >> + qdict_flatten_qlist(qobject_to_qlist(value), target, new_key); >> + } >> + } >> +} >> + >> +static void qdict_flatten_qdict(QDict *qdict, QDict *target, const=20 >> char *prefix) >> { >> QObject *value; >> const QDictEntry *entry, *next; >> @@ -500,8 +533,12 @@ static void qdict_do_flatten(QDict *qdict, QDict=20 >> *target, const char *prefix) >> if (qobject_type(value) =3D=3D 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 =3D true; >> + } else if (qobject_type(value) =3D=3D QTYPE_QLIST) { >> + qdict_flatten_qlist(qobject_to_qlist(value), target, >> + new_key ? new_key : entry->key); >> delete =3D true; >> } else if (prefix) { >> /* All other objects are moved to the target unchanged. */ >> @@ -531,7 +568,7 @@ static void qdict_do_flatten(QDict *qdict, QDict=20 >> *target, const char *prefix) >> */ >> void qdict_flatten(QDict *qdict) >> { >> - qdict_do_flatten(qdict, qdict, NULL); >> + qdict_flatten_qdict(qdict, qdict, NULL); > > If there was an assumption that the top level is a qdict, also assert=20 > that here? Well, the type is already QDict *, therefore I think we are safe to=20 assume it is indeed one =E2=80=93 if that is what you meant. ;-) The question to me would be whether we should be supporting QLists here,=20 too. Since this would make qdict_flatten_qlist() more complex, I think=20 we should not do that until it is required. Thank you, Max