From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35189) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZWDvX-0006jn-BE for qemu-devel@nongnu.org; Sun, 30 Aug 2015 21:30:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZWDvT-0003d3-TS for qemu-devel@nongnu.org; Sun, 30 Aug 2015 21:30:27 -0400 Received: from [59.151.112.132] (port=41773 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZWDvT-0003bn-2i for qemu-devel@nongnu.org; Sun, 30 Aug 2015 21:30:23 -0400 Message-ID: <55E3AE27.4060109@cn.fujitsu.com> Date: Mon, 31 Aug 2015 09:30:15 +0800 From: Yang Hongyang MIME-Version: 1.0 References: <1440583182-5828-1-git-send-email-yanghy@cn.fujitsu.com> <1440583182-5828-11-git-send-email-yanghy@cn.fujitsu.com> <87wpwijb8l.fsf@blackfin.pond.sub.org> <55DE7933.7070706@cn.fujitsu.com> <87eginabbo.fsf@blackfin.pond.sub.org> In-Reply-To: <87eginabbo.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v8 10/11] filter/buffer: update command description and help List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: thuth@redhat.com, zhang.zhanghailiang@huawei.com, lizhijian@cn.fujitsu.com, jasowang@redhat.com, qemu-devel@nongnu.org, mrhines@linux.vnet.ibm.com, Luiz Capitulino , stefanha@redhat.com On 08/28/2015 07:42 PM, Markus Armbruster wrote: > Yang Hongyang writes: > >> On 08/26/2015 11:55 PM, Markus Armbruster wrote: >>> Yang Hongyang writes: >>> >>>> now that we have a buffer netfilter, update the command >>>> description and help. >>>> >>>> Signed-off-by: Yang Hongyang >>>> CC: Luiz Capitulino >>>> CC: Markus Armbruster >>>> --- >>>> 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.