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

[Philippe Mathieu-Daudé]

On 5/14/19 2:18 PM, 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().
> 
> 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"?
> 
> -netdev's helper parameter is seriously underdocumented.  Document or
> deprecate?
> 

I understood the project policy is "deprecate until maintained or
tested"... If not, we might start to think about it :)