[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/2 v2] slirp: Add classless static routes suppo
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH 2/2 v2] slirp: Add classless static routes support to DHCP server |
Date: |
Thu, 8 Mar 2018 14:21:57 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 |
On 03/08/2018 02:07 PM, Benjamin Drung wrote:
Am Donnerstag, den 08.03.2018, 13:46 -0600 schrieb 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
Thanks. I will do that with the next iteration. Patch v2 addressed all
remarks from Samuel Thibault.
At this point, since Samuel is the net maintainer, I'll trust his
judgment on what interface works best; my review is only trying to make
sure we don't bake in a UI mistake at the last minute (although we can
adjust things during soft freeze, if needed).
'*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.
Agreed that ['str'] is nicer. I will update the patch.
The problem is that ['str'] might not work easily for the command line
glue; I'm more familiar with how QMP exposes things than with the
command line parsing, and Markus, who is trying to improve command line
parsing to share more common infrastructure with QMP, might have better
comments on the topic, except that he's on leave for a few weeks and
won't respond until after 2.12 is frozen. Using ['String'] for
consistency is therefore okay, if you can't get ['str'] working quickly.
@@ -1904,7 +1904,7 @@ DEF("netdev", HAS_ARG, QEMU_OPTION_netdev,
" [,ipv6[=on|off]][,ipv6-net=addr[/int]][,ipv6-
host=addr]\n"
Here's an example where we made the command line smart. ipv6-net takes
TWO pieces of information: addr/int; on the QMP side, we spelled it
'*ipv6-prefix':'str' + 'ipv6-prefixlen':'int'. So somewhere in the
command line parsing code for --net (which I'm less familiar with),
there is some glue code taking the compact representation and splitting
it into the more verbose but more direct QMP representation - well, that
is, if we are converting it into QMP form at all (part of the problem is
that our command line and runtime control don't always share code,
although we're trying to get better at that).
+ " [,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": "..." } ]
That's looks good.
Okay, doing that would mean using something like:
{ 'struct': 'RouteEntry', 'data': { 'addr': 'str', '*mask': 'int',
'*gateway': 'str' } }
...
'route': [ 'RouteEntry' ]
(but reuse, rather than inventing a new type, if one of the existing QMP
types already resembles what I proposed for RouteEntry)
The command line can still use route=addr/mask:gateway syntax, parse it
down into components, then compile the QMP array of already-parsed
structs (rather than making QMP take a direct ['String'] that still
needs further parsing). It may take more glue code, but the idea is
that all the glue code should live on the front end, so that the QMP
backend should be easy to work with.
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.
Can you point me to an example where similar parsing is done?
Hopefully my hint about command-line ipv6-net gets you started (as I
said, I'm less familiar with the specifics of net code, so much as
taking the interface point of view here).
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_docum
entation
I can change it, but then the documentation is inconsistent. There
are 75 lines with '-net' in qemu-options.hx, but only two lines
with '--net'.
Yeah, there's that. But hopefully someone will tackle the bite-sized
task to get things consistent, and once they do, leaving fewer places
that still need to be switched is nice. I can live with either spelling
until the bite-sized task is tackled, and am not implying you have to
take on that task yourself, so much as pointing it out so you can choose
if it is worth the nicer spelling for new content, or leaving it as-is
to feel more consistent in the short term.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org