Date: Sat, 9 May 2015 09:47:38 +0200
From: Thomas Huth <thuth@redhat.com>
Message-ID: <20150509094738.13dffc12@thh440s>
In-Reply-To: <87r3qrdyrg.fsf@blackfin.pond.sub.org>
References: <1431077765-6461-1-git-send-email-thuth@redhat.com>
	<87r3qrdyrg.fsf@blackfin.pond.sub.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH] net: Change help text to list -netdev
 instead of -net by default
To: Markus Armbruster <armbru@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>, qemu-devel@nongnu.org, Stefan Hajnoczi <stefanha@redhat.com>, "Michael S. Tsirkin" <mst@redhat.com>

On Fri, 08 May 2015 14:44:51 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Thomas Huth <thuth@redhat.com> writes:
> 
> > Looking at the output of "qemu-system-xxx -help", you easily get
> > the impression that "-net" is the preferred way instead of "-netdev"
> > to specify host network interface, since the "-net" option is
> > omnipresent but the "-netdev" option is only listed as a one-liner
> > at the end. This is ugly since "-net" is considered as legacy and
> > even might be removed one day. Thus, this patch switches the output
> > to explain the host network interfaces with the "-netdev" option
> > instead, moving the legacy "-net" option into some few lines at
> > the end.
> 
> Thanks a lot for tackling this!
> 
> I'm only superficially familiar with this stuff, but that's not a bad
> thing for reviewing help, so here goes.

Thanks for the review, most of your suggestions look really fine, will
include them in the next version of the patch!

[...]
> >      "                (default=" DEFAULT_BRIDGE_INTERFACE ") using the program 'helper'\n"
> >      "                (default=" DEFAULT_BRIDGE_HELPER ")\n"
> >  #endif
> >  #ifdef __linux__
> > -    "-net l2tpv3[,vlan=n][,name=str],src=srcaddr,dst=dstaddr[,srcport=srcport][,dstport=dstport],txsession=txsession[,rxsession=rxsession][,ipv6=on/off][,udp=on/off][,cookie64=on/off][,counter][,pincounter][,txcookie=txcookie][,rxcookie=rxcookie][,offset=offset]\n"
> > +    "-netdev l2tpv3,id=str,src=srcaddr,dst=dstaddr[,srcport=srcport][,dstport=dstport]\n"
> > +    "         [,rxsession=rxsession],txsession=txsession[,ipv6=on/off][,udp=on/off]\n"
> > +    "         [,cookie64=on/off][,counter][,pincounter][,txcookie=txcookie]\n"
> > +    "         [,rxcookie=rxcookie][,offset=offset]\n"
> >      "                connect the VLAN to an Ethernet over L2TPv3 pseudowire\n"
> 
> "connect the *VLAN*"?

Ah, missed to update that string ...

> What about:
> 
>                 configure a network backend with ID 'str'
>                 connected to an Ethernet over L2TPv3 pseudowire

... and this sounds better of course.

> I'm sure you get the idea by now: help always starts with something like
> "configure a [FOO] network backend with ID 'str' [connected thusly].
> Nice and regular.
> 
> More of the same below.

Ack, will update.

> >      "-net dump[,vlan=n][,file=f][,len=n]\n"
> >      "                dump traffic on vlan 'n' to file 'f' (max n bytes per packet)\n"
> >      "-net none       use it alone to have zero network devices. If no -net option\n"
> > -    "                is provided, the default is '-net nic -net user'\n", QEMU_ARCH_ALL)
> > -DEF("netdev", HAS_ARG, QEMU_OPTION_netdev,
> > -    "-netdev ["
> > +    "                is provided, the default is '-net nic -net user'\n"
> > +    "-net ["
> >  #ifdef CONFIG_SLIRP
> >      "user|"
> >  #endif
> > @@ -1551,9 +1561,9 @@ DEF("netdev", HAS_ARG, QEMU_OPTION_netdev,
> >  #ifdef CONFIG_NETMAP
> >      "netmap|"
> >  #endif
> > -    "vhost-user|"
> > -    "socket|"
> > -    "hubport],id=str[,option][,option][,...]\n", QEMU_ARCH_ALL)
> > +    "socket][,vlan=n][,option][,option][,...]\n"
> > +    "                Deprecated way to initialize a host network interface\n"
> > +    "                (use -netdev instead)\n", QEMU_ARCH_ALL)
> 
> Should we add a hint on how to translate -net to -netdev?

I would not include this in the output of -help. -help is just for
getting a quick look at the syntax and not for getting full
documentation, I think. Such a hint should go into the normal doc files
instead (and there you already can see the options side by side).


> > @@ -1572,6 +1582,8 @@ Valid values for @var{type} are
> >  @code{e1000}, @code{smc91c111}, @code{lance} and @code{mcf_fec}.
> >  Not all devices are supported on all targets.  Use @code{-net nic,model=help}
> >  for a list of available devices for your target.
> > +Note that this option is deprecated, NICs should be created with the
> > +@code{-device @var{type},netdev=@var{id}} option instead.
> >  
> >  @item -netdev user,id=@var{id}[,@var{option}][,@var{option}][,...]
> >  @findex -netdev
> 
> The hunks above rearranged -help to show -netdev before -net.  You could
> do the same for the documentation in STEXI..ETEXI.  But the patch is
> valuable even without it.

Ok, here the options are normally listed side by side already. It's
just the "-net nic" option that still sits at the top. But I can move
that to the end of the section too, if you like.

 Thomas