Re: [Qemu-devel] [PATCH v8 10/11] filter/buffer: update command description and help

[Markus Armbruster]

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.

>>
>> 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.

>>>                                                         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.