[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 00/17] introduce OptsVisitor, rebase -net/-ne
From: |
Laszlo Ersek |
Subject: |
Re: [Qemu-devel] [PATCH v2 00/17] introduce OptsVisitor, rebase -net/-netdev parsing |
Date: |
Fri, 13 Jul 2012 21:28:59 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:10.0.5) Gecko/20120601 Thunderbird/10.0.5 |
On 07/13/12 18:46, Luiz Capitulino wrote:
> On Wed, 13 Jun 2012 10:22:31 +0200
> Laszlo Ersek <address@hidden> wrote:
>
>> Inspired by [1], the first half of this series attempts to implement a new
>> visitor that should clean up defining and processing command line options.
>> For a more detailed description, please see "[PATCH 05/17] qapi: introduce
>> OptsVisitor".
>>
>> The second half converts -net/-netdev parsing to the new visitor.
>
> The general approach looks fine to me, I've made comments to individual
> patches
> and have two general comments:
>
> 1. This doesn't build for me:
>
> 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: unknown
> type name ‘QemuOptsList’
> /home/lcapitulino/work/src/qmp-unstable/net/slirp.c:741:5: error: no previous
> prototype for ‘net_slirp_parse_legacy’ [-Werror=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.
>
> 2. I don't think this should go in through qmp's branch because this is more
> about QemuOpts than about QMP. I suggest three alternatives:
>
> - If you're going to go forward and convert more users, then I think
> you should open your own branch, send pull requests etc
>
> - Go through some -net three
>
> - Ask Anthony to apply this directly
>
> I'll, of course, review it though
I think I'll ask Anthony to apply v3 directly.
Thanks for the review!
Laszlo