Re: [Qemu-devel] [PATCH 1/6] qemu-bridge-helper: Fix misuse of isspace()

[Jason Wang]

On 2019/5/14 下午8:18, Markus Armbruster wrote:
> Peter Maydell <address@hidden> writes:
>
> > On Mon, 13 May 2019 at 14:21, Markus Armbruster <address@hidden> wrote:
> > > Perhaps I should do it just for this file while I touch it anyway.  The
> > > question to ask: should parse_acl_file() obey the locale for whitespace
> > > recognition?
> > I vote for "no".
> >
> > Q: do we document the format of the ACL file anywhere ?
> Support for it was added in commit bdef79a2994, v1.1.  Just code, no
> documentation.
>
> Grepping for qemu-bridge-helper finds just qemu-options.hx.  Contains
> -help output and some .texi that goes into qemu-doc and the manual page.
> None of it mentions how qemu-bridge-helper is run, or the ACL file
> feature, let alone what its format might be.
>
> I'm afraid all we have is the commit message.  Which doesn't really
> define the file format, it merely gives a bunch of examples.
>
> As far as I can tell, qemu-bridge-helper is for use with -netdev tap and
> -netdev bridge.
>
> Both variations of -netdev call net_bridge_run_helper() to run the
> helper.  First argument is -netdev parameter "helper", default usually
> "$prefix/libexec/qemu-bridge-helper".  Second argument is parameter
> "br", default "br0".
>
> If @helper contains space or tab, net_bridge_run_helper() guesses its a
> full command, else it guesses its the name of the executable.  Bad
> magic.
>
> If it guesses name of executable, it execv()s this executable with
> arguments "--use-vnet", "--fd=FD", "address@hidden".
>
> If it guesses full command, it appends "--use-vnet --fd=FD", where FD is
> the helper's half of the socketpair used to connect QEMU and the helper.
> It further appends "address@hidden", unless @helper contains "--br=".
> More bad magic.
>
> It executes the resulting string with sh -c.  Magic cherry on top.
>
> When the helper fails, netdev creation fails.
>
> The helper we ship with QEMU unconditionally tries to read
> "$prefix/etc/bridge.conf".  Fatal error if this file doesn't exist.
> Errors in this file are fatal.  Errors in files it includes are not
> fatal; instead, the remainder of the erroneous file is ignored.
> *Boggle*
>
> As far as I can tell, libvirt runs qemu-bridge-helper itself (Paolo's
> commit 2d80fbb14df).  Makes sense, because running QEMU with the
> necessary privileges would be unwise, and so would be letting it execute
> setuid helpers.  Also bypasses the bad magic in QEMU's
> net_bridge_run_helper().


I don't notice this before. Is this only for the convenience of 
development? I guess libvirt should have native support like adding port 
to bridge/OVS without the help any external command or script.


> qemu-bridge-helper should have a manual page, and its handling of errors
> in ACL include files needs work.  There's probably more; I just glanced
> at it.  I'm not volunteering, though.  It lacks a maintainer.  Should we
> add it to Jason's "Network device backends"?


Yes.


> -netdev's helper parameter is seriously underdocumented.  Document or
> deprecate?


I believe management should only use fd parameter of TAP. If we have 
other, it should be a duplication. So I suggest to deprecate the bridge 
helper and -netdev bridge.

Thanks