From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:40999) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SplWo-0005xp-Ic for qemu-devel@nongnu.org; Fri, 13 Jul 2012 15:27:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SplWn-0007kc-Fz for qemu-devel@nongnu.org; Fri, 13 Jul 2012 15:27:50 -0400 Received: from mx1.redhat.com ([209.132.183.28]:4303) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SplWn-0007kW-7o for qemu-devel@nongnu.org; Fri, 13 Jul 2012 15:27:49 -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 q6DJRm8W011525 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Fri, 13 Jul 2012 15:27:48 -0400 Message-ID: <500076FB.4070403@redhat.com> Date: Fri, 13 Jul 2012 21:28:59 +0200 From: Laszlo Ersek MIME-Version: 1.0 References: <1339575768-2557-1-git-send-email-lersek@redhat.com> <20120713134638.3986ff09@doriath.home> In-Reply-To: <20120713134638.3986ff09@doriath.home> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v2 00/17] introduce OptsVisitor, rebase -net/-netdev parsing List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino Cc: qemu-devel@nongnu.org On 07/13/12 18:46, Luiz Capitulino wrote: > On Wed, 13 Jun 2012 10:22:31 +0200 > Laszlo Ersek wrote: >=20 >> Inspired by [1], the first half of this series attempts to implement a= new >> visitor that should clean up defining and processing command line opti= ons. >> For a more detailed description, please see "[PATCH 05/17] qapi: intro= duce >> OptsVisitor". >> >> The second half converts -net/-netdev parsing to the new visitor. >=20 > The general approach looks fine to me, I've made comments to individual= patches > and have two general comments: >=20 > 1. This doesn't build for me: >=20 > In file included from /home/lcapitulino/work/src/qmp-unstable/net/slirp= .c:24:0: > /home/lcapitulino/work/src/qmp-unstable/net/slirp.h:41:28: error: unkno= wn type name =E2=80=98QemuOptsList=E2=80=99 > /home/lcapitulino/work/src/qmp-unstable/net/slirp.c:741:5: error: no pr= evious prototype for =E2=80=98net_slirp_parse_legacy=E2=80=99 [-Werror=3D= missing-prototypes] > cc1: all warnings being treated as errors > make: *** [net/slirp.o] Error 1 > make: *** Waiting for unfinished jobs.... Okay this took some time to track down. When I posted v2, it was based on 7677e24f in my clone. I made a mistake in 17/17, in "net/slirp.h": I removed "qemu-option.h" after conversion was finished, because I didn't notice net_slirp_parse_legacy() continued to depend on QemuOptsList. The error went unnoticed because @ 7677e24f this was the relevant #include tree, rooted at net/slirp.h: net/slirp.h qapi-types.h qapi/qapi-types-core.h monitor.h qemu-char.h qemu-option.h <--- block.h qemu-aio.h qemu-char.h qemu-option.h <--- qemu-option.h <--- Then Paolo's patch was committed as ad608da5 ("qmp: do not include monitor.h from qapi-types-core.h"). The above tree was cut at "monitor.h", severing all three marked paths. I must reinclude "qemu-option.h" and squash the change into 17/17. >=20 > 2. I don't think this should go in through qmp's branch because this i= s more > about QemuOpts than about QMP. I suggest three alternatives: >=20 > - If you're going to go forward and convert more users, then I th= ink > you should open your own branch, send pull requests etc >=20 > - Go through some -net three >=20 > - Ask Anthony to apply this directly >=20 > I'll, of course, review it though I think I'll ask Anthony to apply v3 directly. Thanks for the review! Laszlo