Re: [Qemu-devel] [PATCH v2 00/17] introduce OptsVisitor, rebase -net/-netdev parsing

[Laszlo Ersek]

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