Re: [Qemu-devel] [PATCH 2/2 v2] slirp: Add classless static routes support to DHCP server

[Eric Blake]

On 03/08/2018 12:57 PM, Benjamin Drung wrote:
> This patch will allow the user to specify classless static routes for
> the replies from the built-in DHCP server.
>
> Signed-off-by: Benjamin Drung <address@hidden>
> ---

For future patches, when sending a v2, it's best to document here (after 
the --- separator) what changed from v1.  It's also a good idea to send 
a fresh thread rather than tying your v2 in-reply-to your v1, so that it 
doesn't get buried in an old conversation.

More submission hints at https://wiki.qemu.org/Contribute/SubmitAPatch

>   net/slirp.c      | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++++---
>   qapi/net.json    |  4 ++++
>   qemu-options.hx  | 14 +++++++++++-
>   slirp/bootp.c    | 20 +++++++++++++++++
>   slirp/bootp.h    |  2 ++
>   slirp/libslirp.h |  9 +++++++-
>   slirp/slirp.c    |  7 +++++-
>   slirp/slirp.h    |  2 ++
>   8 files changed, 120 insertions(+), 6 deletions(-)

Reviewing just the QMP interface:


> +++ b/qapi/net.json
> @@ -163,6 +163,9 @@
>   # @domainname: guest-visible domain name of the virtual nameserver
>   #              (since 2.12)
>   #
> +# @route: guest-visible static classless route of the virtual nameserver
> +#         (since 2.12)
> +#
>   # @ipv6-prefix: IPv6 network prefix (default is fec0::) (since
>   #               2.6). The network prefix is given in the usual
>   #               hexadecimal IPv6 address notation.
> @@ -201,6 +204,7 @@
>       '*dns':       'str',
>       '*dnssearch': ['String'],
>       '*domainname': 'str',
> +    '*route':     ['String'],

I know we've used ['String'] for previous members, but that's rather 
heavyweight - it transmits over QMP as:

"dnssearch": [ { "str": "foo" }, { "str": "bar" } ]

Nicer is ['str'], which transmits as:

"route": [ "foo", "bar" ]

so the question boils down to whether cross-member consistency is more 
important than making your additions concise.

> +++ b/qemu-options.hx
> @@ -1904,7 +1904,7 @@ DEF("netdev", HAS_ARG, QEMU_OPTION_netdev,
>       "         [,ipv6[=on|off]][,ipv6-net=addr[/int]][,ipv6-host=addr]\n"
>       "         [,restrict=on|off][,hostname=host][,dhcpstart=addr]\n"
>       "         
> [,dns=addr][,ipv6-dns=addr][,dnssearch=domain][,domainname=domain]\n"
> -    "         [,tftp=dir][,bootfile=f][,hostfwd=rule][,guestfwd=rule]"
> +    "         
> [,route=addr/mask[:gateway]][,tftp=dir][,bootfile=f][,hostfwd=rule][,guestfwd=rule]"

Urgh - your QMP interface HAS to be further parsed to get to the useful 
information.  While it's nice to have compact syntax on the command 
line, it is really worth thinking about making information easier to 
consume (that is, NO further parsing required once the information is in 
JSON format).  Would it be any better to send things over the wire as:

"route": [ { "addr": "...", "mask": 24, "gateway": "..." } ]

instead of cramming all the information into a single string?  But based 
on the way this also maps to the command line, you may not have a choice 
without a lot more code complexity.

>   #ifndef _WIN32
>                                                "[,smb=dir[,smbserver=addr]]\n"
>   #endif
> @@ -2137,6 +2137,18 @@ qemu -net 
> user,dnssearch=mgmt.example.org,dnssearch=example.org [...]
>   @item address@hidden
>   Specifies the client domain name reported by the built-in DHCP server.
>   
> address@hidden address@hidden/@var{mask}[:@var{gateway}]
> +Provides an entry for the classless static routes list sent by the built-in
> +DHCP server. More than one route can be transmitted by specifying
> +this option multiple times. If supported, this will cause the guest to
> +automatically set the given static routes instead of the given default gateway.
> +If @var{gateway} is not specified, the default gateway will be used.
> +
> +Example:
> address@hidden
> +qemu -net user,route=10.0.2.0/24,route=192.168.0.0/16 [...]
> address@hidden example

Can we please spell that '--net', along the lines of 
https://wiki.qemu.org/BiteSizedTasks#Consistent_option_usage_in_documentation

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org