|
| From: | Yang Hongyang |
| Subject: | Re: [Qemu-devel] [PATCH v8 10/11] filter/buffer: update command description and help |
| Date: | Mon, 31 Aug 2015 09:30:15 +0800 |
| User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 |
On 08/28/2015 07:42 PM, Markus Armbruster wrote:
Yang Hongyang <address@hidden> writes:On 08/26/2015 11:55 PM, Markus Armbruster wrote:Yang Hongyang <address@hidden> writes:now that we have a buffer netfilter, update the command description and help. Signed-off-by: Yang Hongyang <address@hidden> CC: Luiz Capitulino <address@hidden> CC: Markus Armbruster <address@hidden> --- v8: add more description for the filter to the TEXI section --- hmp-commands.hx | 2 +- qemu-options.hx | 18 +++++++++++++++++- qmp-commands.hx | 2 +- 3 files changed, 19 insertions(+), 3 deletions(-) diff --git a/hmp-commands.hx b/hmp-commands.hx index 902e2d1..63177a8 100644 --- a/hmp-commands.hx +++ b/hmp-commands.hx @@ -1255,7 +1255,7 @@ ETEXI { .name = "netfilter_add", .args_type = "netfilter:O", - .params = "[type],id=str,netdev=str[,chain=in|out|all,prop=value][,...]", + .params = "[buffer],id=str,netdev=str[,chain=in|out|all,prop=value][,...]", .help = "add netfilter", .mhandler.cmd = hmp_netfilter_add, .command_completion = netfilter_add_completion,This change looks odd. Actually, params look odd before and after the change :) I suspect you're trying to follow netdev_add precedence. Its params: .params = "[user|tap|socket|vde|bridge|hubport|netmap|vhost-user],id=str[,prop=value][,...]", Neglects to mention the long form type=, but that's pretty common. Uses square brackets both for grouping and for optionalness. We suck. Users can probably figure out that the first [ ] is just grouping, and the others are optionalness. Your params are even more confusing, because the first [ ] is again grouping, but there's just one alternative: buffer. What about not simply writing .params = "buffer,id=str,netdev=str[,chain=in|out|all,prop=value][,...]", for now?Sure, I agree it is confusing when it has only one alternative now...When we added more filters, we can change it later.diff --git a/qemu-options.hx b/qemu-options.hx index 0d52d02..390e055 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -1575,7 +1575,10 @@ DEF("net", HAS_ARG, QEMU_OPTION_net, "socket][,vlan=n][,option][,option][,...]\n" " old way to initialize a host network interface\n" " (use the -netdev option if possible instead)\n", QEMU_ARCH_ALL) -DEF("netfilter", HAS_ARG, QEMU_OPTION_netfilter, "", QEMU_ARCH_ALL) +DEF("netfilter", HAS_ARG, QEMU_OPTION_netfilter, + "-netfilter buffer,id=str,netdev=str[,chain=in|out|all,interval=n]\n" + " buffer netdev in/out packets. if interval provided, will release\n" + " packets by interval. interval scale: microsecond\n", QEMU_ARCH_ALL)If I understand your intent correctly, "interval" is the amount of time to delay packets. What about calling it "delay"?You can take it as the amount of time to delay packets, but for other usecase like MC, you can also take it as an interval to release packets, that is to say, to release packets every {interval} time.Perhaps a less terse explanation would make that easier to understand. Try to do it for STEXI..ETEXI, and then we can see whether we still want to make the help text less terse, too.
Ok, thanks!
Suggest something like buffer network packets, delaying them for 'delay' microseconds (default 0us)STEXI @item -net nic[,address@hidden,address@hidden,address@hidden [,address@hidden,address@hidden,address@hidden @findex -net @@ -1990,6 +1993,19 @@ libpcap, so it can be analyzed with tools such as tcpdump or Wireshark. Indicate that no network devices should be configured. It is used to override the default configuration (@option{-net nic -net user}) which is activated if no @option{-net} options are provided. + address@hidden -netfilter buffer,address@hidden,address@hidden,address@hidden/out/all}][,address@hidden'n' is an unusual choice for a time delay. What about 't', 'dt', or 'delay'?'t' is better, thanks!+Buffer netdev @var{netdevid} input or output packets.Are there any other kinds of packets?No, only input/output packets now.Recommend to say just "network packets" then.
Ok, thanks.
if interval @var{n} +provided, will release packets by interval. interval scale: microsecond.Please start sentences with a capital letter. Suggest something like Packets are delayed by @var{n} microseconds. Defaults to 0us, i.e. no delay.+ +chain @var{in/out/all} is an option that can be applied to any netfilters, if +not provided, default is @option{all}."to any netfilter" (singular) Drop "if not provided,"Ok, thanks!+ address@hidden means this filter will receive packets sent to the netdev + address@hidden means this filter will receive packets sent from the netdev + address@hidden means this filter will receive packets both sent to/from the netdev ETEXI STEXI diff --git a/qmp-commands.hx b/qmp-commands.hx index 4f0dc98..9419a6f 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -947,7 +947,7 @@ Arguments: Example: -> { "execute": "netfilter_add", - "arguments": { "type": "type", "id": "nf0", + "arguments": { "type": "buffer", "id": "nf0", "netdev": "bn", "chain": "in" } } <- { "return": {} }Does the example before this patch actually work?No, there's only type=buffer now.The commit message of the patch that adds the still non-functional command should explain that it doesn't yet work, because it's still merely infrastructure without an actual user, and that you'll add the first user shortly.
Will do, thanks!
.
-- Thanks, Yang.
| [Prev in Thread] | Current Thread | [Next in Thread] |