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

[Markus Armbruster]

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?

> 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"?

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'?

> +Buffer netdev @var{netdevid} input or output packets.

Are there any other kinds of packets?

>                                                        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,"

> +
> 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?