qemu-devel
[Top][All Lists]
Advanced

[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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]