Re: [Qemu-devel] [PATCH v8 03/11] netfilter: add netfilter_{add|del} commands

[Markus Armbruster <armbru@redhat.com>]

Only reviewing QAPI/QMP and HMP interface parts for now.

I apologize for not having reviewed this series earlier.  v8 is awfully
late for the kind of review comments I have.

Yang Hongyang <yanghy@cn.fujitsu.com> writes:

> add netfilter_{add|del} commands
> This is mostly the same with netdev_{add|del} commands.
>
> When we delete the netdev, we also delete the netfilter object
> attached to it, because if the netdev is removed, the filters
> which attached to it is useless.
>
> Signed-off-by: Yang Hongyang <yanghy@cn.fujitsu.com>
> CC: Luiz Capitulino <lcapitulino@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> ---
> v7: error msg fix
>     move qmp_opts_del() into qemu_del_net_filter()
> v6: add multiqueue support (qemu_del_net_filter)
> v5: squash "net: delete netfilter object when delete netdev"
> ---
>  hmp-commands.hx      |  30 +++++++++++++++
>  hmp.c                |  29 +++++++++++++++
>  hmp.h                |   4 ++
>  include/net/filter.h |   3 ++
>  monitor.c            |  33 +++++++++++++++++
>  net/filter.c         | 101 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  net/net.c            |   7 ++++
>  qapi-schema.json     |  47 ++++++++++++++++++++++++
>  qmp-commands.hx      |  57 +++++++++++++++++++++++++++++
>  9 files changed, 310 insertions(+), 1 deletion(-)
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index d3b7932..902e2d1 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1253,6 +1253,36 @@ Remove host network device.
>  ETEXI
>  
>      {
> +        .name       = "netfilter_add",
> +        .args_type  = "netfilter:O",
> +        .params     = "[type],id=str,netdev=str[,chain=in|out|all,prop=value][,...]",
> +        .help       = "add netfilter",
> +        .mhandler.cmd = hmp_netfilter_add,

Supporting completion from the start is a nice touch.

> +        .command_completion = netfilter_add_completion,
> +    },
> +
> +STEXI
> +@item netfilter_add
> +@findex netfilter_add
> +Add netfilter.
> +ETEXI

Awfully terse for a user manual.  Please try to follow the good examples
instead of the bad examples in this file :)

> +
> +    {
> +        .name       = "netfilter_del",
> +        .args_type  = "id:s",
> +        .params     = "id",
> +        .help       = "remove netfilter",
> +        .mhandler.cmd = hmp_netfilter_del,
> +        .command_completion = netfilter_del_completion,
> +    },
> +
> +STEXI
> +@item netfilter_del
> +@findex netfilter_del
> +Remove netfilter.
> +ETEXI
> +
> +    {

Likewise.

>          .name       = "object_add",
>          .args_type  = "object:O",
>          .params     = "[qom-type=]type,id=str[,prop=value][,...]",
[...]
> diff --git a/qapi-schema.json b/qapi-schema.json
> index d7fb578..9d97c21 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2537,6 +2537,53 @@
>      'opts': 'NetClientOptions' } }
>  
>  ##
> +# @netfilter_add:
> +#
> +# Add a netfilter.
> +#
> +# @type: the type of netfilter.
> +#
> +# @id: the name of the new netfilter.
> +#
> +# @netdev: the name of the netdev which this filter will be attached to.
> +#
> +# @chain: #optional accept "in","out","all", if not specified, default is "all"
> +#         "in" means this filter will receive packets sent to the @netdev
> +#         "out" means this filter will receive packets sent from the @netdev
> +#         "all" means this filter will receive packets both sent to/from
> +#               the @netdev
> +#
> +# @props: #optional a list of properties to be passed to the netfilter in
> +#         the format of 'name=value'
> +#
> +# Since: 2.5
> +#
> +# Returns: Nothing on success
> +#          If @type is not a valid netfilter, DeviceNotFound
> +##
> +{ 'command': 'netfilter_add',
> +  'data': {
> +    'type': 'str',
> +    'id':   'str',
> +    'netdev': 'str',
> +    '*chain': 'str',
> +    '*props': '**'}, 'gen': false }

I figure you're merely following netdev_add precedence here (can't fault
you for that), but netdev_add cheats, and we shouldn't add more cheats.

First, 'gen': false is best avoided.  It suppresses the generated
marshaller, and that lets you cheat.  There are cases where we can't do
without, but I don't think this is one.

When we suppress the generated marshaller, 'data' is at best a
declaration of intent; the code can do something else entirely.  For
instance, netdev_add declares

    { 'command': 'netdev_add',
      'data': {'type': 'str', 'id': 'str', '*props': '**'},
      'gen': false }

but the '*props' part is a bald-faced lie: it doesn't take a 'props'
argument.  See
http://lists.gnu.org/archive/html/qemu-devel/2015-08/msg00460.html
and maybe even slides 37-38 of
https://events.linuxfoundation.org/sites/events/files/slides/armbru-qemu-introspection.pdf

I didn't check your code, but I suspect yours is a lie, too.

I intend to clean up netdev_add, hopefully soon.

You should use a proper QAPI data type here.  I guess the flat union I
sketched in my reply to PATCH 2 would do nicely, except we don't support
commands with union type data, yet.  I expect to add support to clean up
netdev_del.

If you don't want to wait for that (understandable!), then I suggest you
keep 'NetFilter' a struct type for now, use it as command data here, and
we convert it to a flat union later.  Must be done before the release,
to avoid backward incompatibility.

Then this becomes something like:

    { 'command': 'netfilter-add', 'data': 'NetFilter' }

If you need the command to take arguments you don't want to put into
NetFilter for some reason, I can help you achieve that cleanly.

> +
> +##
> +# @netfilter_del:
> +#
> +# Remove a netfilter.
> +#
> +# @id: the name of the netfilter to remove
> +#
> +# Returns: Nothing on success
> +#          If @id is not a valid netfilter, DeviceNotFound
> +#
> +# Since: 2.5
> +##
> +{ 'command': 'netfilter_del', 'data': {'id': 'str'} }
> +
> +##
>  # @NetFilterOptions
>  #
>  # A discriminated record of network filters.
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index ba630b1..4f0dc98 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -926,6 +926,63 @@ Example:
>  EQMP
>  
>      {
> +        .name       = "netfilter_add",

'-' instead of '_' in new QMP commands, please.

> +        .args_type  = "netfilter:O",

Again, you're merely following netdev_add precedence here, but args_type
'O' is problematic, and should not be used in new code.  I hope to get
rid of it entirely.  Easiest for now is probably something like
"options:q".  Details depend on how exactly you do the schema.

> +        .mhandler.cmd_new = qmp_netfilter_add,
> +    },
> +
> +SQMP
> +netfilter_add
> +----------
> +
> +Add netfilter.
> +
> +Arguments:
> +
> +- "type": the filter type (json-string)
> +- "id": the netfilter's ID, must be unique (json-string)
> +- "netdev": the netdev's ID which this filter will be attached to(json-string)
> +- filter options
> +
> +Example:
> +
> +-> { "execute": "netfilter_add",
> +                "arguments": { "type": "type", "id": "nf0",
> +                               "netdev": "bn",
> +                               "chain": "in" } }
> +<- { "return": {} }
> +
> +Note: The supported filter options are the same ones supported by the
> +      '-netfilter' command-line argument, which are listed in the '-help'
> +      output or QEMU's manual
> +
> +EQMP
> +
> +    {
> +        .name       = "netfilter_del",
> +        .args_type  = "id:s",
> +        .mhandler.cmd_new = qmp_marshal_input_netfilter_del,
> +    },
> +
> +SQMP
> +netfilter_del
> +----------
> +
> +Remove netfilter.
> +
> +Arguments:
> +
> +- "id": the netfilter's ID, must be unique (json-string)
> +
> +Example:
> +
> +-> { "execute": "netfilter_del", "arguments": { "id": "nf0" } }
> +<- { "return": {} }
> +
> +
> +EQMP
> +
> +    {
>          .name       = "object-add",
>          .args_type  = "qom-type:s,id:s,props:q?",
>          .mhandler.cmd_new = qmp_object_add,