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 <yan...@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 <yan...@cn.fujitsu.com>
> CC: Luiz Capitulino <lcapitul...@redhat.com>
> CC: Markus Armbruster <arm...@redhat.com>
> CC: Eric Blake <ebl...@redhat.com>
> Reviewed-by: Thomas Huth <th...@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,
