[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:

          qemu-option.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!

reply via email to

[Prev in Thread] Current Thread [Next in Thread]