On 08/28/2015 07:42 PM, Markus Armbruster wrote:
Yang Hongyang <yan...@cn.fujitsu.com> writes:
On 08/26/2015 11:55 PM, Markus Armbruster wrote:
Yang Hongyang <yan...@cn.fujitsu.com> writes:
now that we have a buffer netfilter, update the command
description and help.
Signed-off-by: Yang Hongyang <yan...@cn.fujitsu.com>
CC: Luiz Capitulino <lcapitul...@redhat.com>
CC: Markus Armbruster <arm...@redhat.com>
---
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[,vlan=@var{n}][,macaddr=@var{mac}][,model=@var{type}]
[,name=@var{name}][,addr=@var{addr}][,vectors=@var{v}]
@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.
+
+@item -netfilter
buffer,id=@var{id},netdev=@var{netdevid}[,chain=@var{in/out/all}][,interval=@var{n}]
'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!
+
+@option{in} means this filter will receive packets sent to the netdev
+
+@option{out} means this filter will receive packets sent from the netdev
+
+@option{all} 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.
