| Message ID | 1440583182-5828-4-git-send-email-yanghy@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
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,
On 08/26/2015 09:17 AM, Markus Armbruster wrote: > 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. And I've also been behind the curve, intending to review this since it touches API, but not getting there yet. >> +## >> +{ '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. In fact, I've already proposed adding such support: http://thread.gmane.org/gmane.comp.emulators.qemu/356265/focus=356291 > > 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' } or use NetFilter as a union, but have the command be: { 'command': 'netfilter-add', 'data': { 'data': 'NetFilter' } } where you have to pass an extra layer of nesting at the QMP layer. > > 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. In fact, having the 'NetFilter' union be a single argument of a larger struct makes that larger struct the nice place to stick any additional arguments that don't need to be part of the union. > >> + >> +## >> +# @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'} } Please name new QMP commands with '-' not '_'; this should be 'netfilter-del'.
Eric Blake <eblake@redhat.com> writes: > On 08/26/2015 09:17 AM, Markus Armbruster wrote: >> 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. > > And I've also been behind the curve, intending to review this since it > touches API, but not getting there yet. > > >>> +## >>> +{ '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. > > In fact, I've already proposed adding such support: > > http://thread.gmane.org/gmane.comp.emulators.qemu/356265/focus=356291 In my review queue. Which has become sickeningly long again... >> >> 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' } > > or use NetFilter as a union, but have the command be: > > { 'command': 'netfilter-add', > 'data': { 'data': 'NetFilter' } } > > where you have to pass an extra layer of nesting at the QMP layer. > >> >> 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. > > In fact, having the 'NetFilter' union be a single argument of a larger > struct makes that larger struct the nice place to stick any additional > arguments that don't need to be part of the union. To make progress, I suggest you try the following: 1. Make NetFilter a flat union, as I suggested in my review of PATCH 2. 2. Use Eric's idea above, because it avoids the dependency on code that's still under review. Drawback: extra layer of nesting. Ugly, but not the end of the world, and we still have a chance to peel it off before the next release. [...]
On 08/28/2015 07:37 PM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> On 08/26/2015 09:17 AM, Markus Armbruster wrote: >>> 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. >> >> And I've also been behind the curve, intending to review this since it >> touches API, but not getting there yet. >> >> >>>> +## >>>> +{ '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. >> >> In fact, I've already proposed adding such support: >> >> http://thread.gmane.org/gmane.comp.emulators.qemu/356265/focus=356291 > > In my review queue. Which has become sickeningly long again... > >>> >>> 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' } >> >> or use NetFilter as a union, but have the command be: >> >> { 'command': 'netfilter-add', >> 'data': { 'data': 'NetFilter' } } >> >> where you have to pass an extra layer of nesting at the QMP layer. >> >>> >>> 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. >> >> In fact, having the 'NetFilter' union be a single argument of a larger >> struct makes that larger struct the nice place to stick any additional >> arguments that don't need to be part of the union. > > To make progress, I suggest you try the following: > > 1. Make NetFilter a flat union, as I suggested in my review of PATCH 2. > > 2. Use Eric's idea above, because it avoids the dependency on code > that's still under review. > > Drawback: extra layer of nesting. Ugly, but not the end of the world, > and we still have a chance to peel it off before the next release. Thanks for the explanation, I will try to see if I can fully understand your point. > > [...] > . >
Yang Hongyang <yanghy@cn.fujitsu.com> writes: > On 08/28/2015 07:37 PM, Markus Armbruster wrote: [...] >> To make progress, I suggest you try the following: >> >> 1. Make NetFilter a flat union, as I suggested in my review of PATCH 2. >> >> 2. Use Eric's idea above, because it avoids the dependency on code >> that's still under review. >> >> Drawback: extra layer of nesting. Ugly, but not the end of the world, >> and we still have a chance to peel it off before the next release. > > Thanks for the explanation, I will try to see if I can fully understand > your point. If you have questions on the QAPI part, Eric and I will be happy to answer them.
On 08/31/2015 03:08 PM, Markus Armbruster wrote: > Yang Hongyang <yanghy@cn.fujitsu.com> writes: > >> On 08/28/2015 07:37 PM, Markus Armbruster wrote: > [...] >>> To make progress, I suggest you try the following: >>> >>> 1. Make NetFilter a flat union, as I suggested in my review of PATCH 2. >>> >>> 2. Use Eric's idea above, because it avoids the dependency on code >>> that's still under review. >>> >>> Drawback: extra layer of nesting. Ugly, but not the end of the world, >>> and we still have a chance to peel it off before the next release. >> >> Thanks for the explanation, I will try to see if I can fully understand >> your point. > > If you have questions on the QAPI part, Eric and I will be happy to > answer them. Thanks a lot for the help! Sorry that I don't know much about the QAPI part, I have a question, in previous reply, Eric suggested: > > Then this becomes something like: > > { 'command': 'netfilter-add', 'data': 'NetFilter' } or use NetFilter as a union, but have the command be: { 'command': 'netfilter-add', 'data': { 'data': 'NetFilter' } } where you have to pass an extra layer of nesting at the QMP layer. What do you mean by pass an extra layer of nesting? I've already switched to flat union as you suggested: { 'struct': 'NetFilterDummyOptions', 'data': { } } { 'enum': 'NetFilterType', 'data': ['dummy'] } { 'struct': 'NetFilterBase', 'data': { 'id': 'str', 'netdev': 'str', '*chain': 'str', 'type': 'NetFilterType' } } { 'union': 'NetFilter', 'base': 'NetFilterBase', 'discriminator': 'type', 'data': { 'dummy': 'NetFilterDummyOptions' } } > . >
On 08/31/2015 03:01 AM, Yang Hongyang wrote: > > Sorry that I don't know much about the QAPI part, I have a question, in > previous reply, Eric suggested: > > > > Then this becomes something like: > > > > { 'command': 'netfilter-add', 'data': 'NetFilter' } > If we do this (which requires pending patches to be flushed), then the user specifies the following QMP: { "execute":"netfilter-add", "arguments":{ "id":"abc", "netdev":"def", "type":"dummy" }} > or use NetFilter as a union, but have the command be: > > { 'command': 'netfilter-add', > 'data': { 'data': 'NetFilter' } } This approach would work right now without waiting for pending qapi commits, but the QMP command would look like: { "execute":"netfilter-add", "arguments":{ "data":{ "id":"abc", "netdev":"def", "type":"dummy" }}} > > where you have to pass an extra layer of nesting at the QMP layer. > > What do you mean by pass an extra layer of nesting? The fact that I had to pass "arguments":{"data":{...}}, thereby nesting the real options inside another relatively pointless data wrapper. > > I've already switched to flat union as you suggested: > > { 'struct': 'NetFilterDummyOptions', > 'data': { } } > > { 'enum': 'NetFilterType', > 'data': ['dummy'] } > > { 'struct': 'NetFilterBase', > 'data': { > 'id': 'str', > 'netdev': 'str', > '*chain': 'str', > 'type': 'NetFilterType' } } > > { 'union': 'NetFilter', > 'base': 'NetFilterBase', > 'discriminator': 'type', > 'data': { > 'dummy': 'NetFilterDummyOptions' } } Looks reasonable for a first round. Some of the pending qapi commits may allow us to further simplify things to not be quite so verbose, but that doesn't stop us from using this now.
On 08/31/2015 10:53 PM, Eric Blake wrote: > On 08/31/2015 03:01 AM, Yang Hongyang wrote: > >> >> Sorry that I don't know much about the QAPI part, I have a question, in >> previous reply, Eric suggested: >> > >> > Then this becomes something like: >> > >> > { 'command': 'netfilter-add', 'data': 'NetFilter' } >> > > If we do this (which requires pending patches to be flushed), then the > user specifies the following QMP: > > { "execute":"netfilter-add", > "arguments":{ "id":"abc", "netdev":"def", "type":"dummy" }} > >> or use NetFilter as a union, but have the command be: >> >> { 'command': 'netfilter-add', >> 'data': { 'data': 'NetFilter' } } > > This approach would work right now without waiting for pending qapi > commits, but the QMP command would look like: > > { "execute":"netfilter-add", > "arguments":{ "data":{ "id":"abc", "netdev":"def", "type":"dummy" }}} > >> >> where you have to pass an extra layer of nesting at the QMP layer. >> >> What do you mean by pass an extra layer of nesting? > > The fact that I had to pass "arguments":{"data":{...}}, thereby nesting > the real options inside another relatively pointless data wrapper. Now I understand, with the flat union, all I need to do now is to specify the command schema like: { 'command': 'netfilter-add', 'data': { 'data': 'NetFilter' } } and use the qmp command like you noted above. Thanks a lot Eric! > >> >> I've already switched to flat union as you suggested: >> >> { 'struct': 'NetFilterDummyOptions', >> 'data': { } } >> >> { 'enum': 'NetFilterType', >> 'data': ['dummy'] } >> >> { 'struct': 'NetFilterBase', >> 'data': { >> 'id': 'str', >> 'netdev': 'str', >> '*chain': 'str', >> 'type': 'NetFilterType' } } >> >> { 'union': 'NetFilter', >> 'base': 'NetFilterBase', >> 'discriminator': 'type', >> 'data': { >> 'dummy': 'NetFilterDummyOptions' } } > > Looks reasonable for a first round. Some of the pending qapi commits > may allow us to further simplify things to not be quite so verbose, but > that doesn't stop us from using this now. >
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, + .command_completion = netfilter_add_completion, + }, + +STEXI +@item netfilter_add +@findex netfilter_add +Add netfilter. +ETEXI + + { + .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 + + { .name = "object_add", .args_type = "object:O", .params = "[qom-type=]type,id=str[,prop=value][,...]", diff --git a/hmp.c b/hmp.c index dcc66f1..09e3cda 100644 --- a/hmp.c +++ b/hmp.c @@ -15,6 +15,7 @@ #include "hmp.h" #include "net/net.h" +#include "net/filter.h" #include "net/eth.h" #include "sysemu/char.h" #include "sysemu/block-backend.h" @@ -1599,6 +1600,34 @@ void hmp_netdev_del(Monitor *mon, const QDict *qdict) hmp_handle_error(mon, &err); } +void hmp_netfilter_add(Monitor *mon, const QDict *qdict) +{ + Error *err = NULL; + QemuOpts *opts; + + opts = qemu_opts_from_qdict(qemu_find_opts("netfilter"), qdict, &err); + if (err) { + goto out; + } + + netfilter_add(opts, &err); + if (err) { + qemu_opts_del(opts); + } + +out: + hmp_handle_error(mon, &err); +} + +void hmp_netfilter_del(Monitor *mon, const QDict *qdict) +{ + const char *id = qdict_get_str(qdict, "id"); + Error *err = NULL; + + qmp_netfilter_del(id, &err); + hmp_handle_error(mon, &err); +} + void hmp_object_add(Monitor *mon, const QDict *qdict) { Error *err = NULL; diff --git a/hmp.h b/hmp.h index 0cf4f2a..a21dbbb 100644 --- a/hmp.h +++ b/hmp.h @@ -85,6 +85,8 @@ void hmp_device_del(Monitor *mon, const QDict *qdict); void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict); void hmp_netdev_add(Monitor *mon, const QDict *qdict); void hmp_netdev_del(Monitor *mon, const QDict *qdict); +void hmp_netfilter_add(Monitor *mon, const QDict *qdict); +void hmp_netfilter_del(Monitor *mon, const QDict *qdict); void hmp_getfd(Monitor *mon, const QDict *qdict); void hmp_closefd(Monitor *mon, const QDict *qdict); void hmp_sendkey(Monitor *mon, const QDict *qdict); @@ -112,6 +114,8 @@ void chardev_add_completion(ReadLineState *rs, int nb_args, const char *str); void set_link_completion(ReadLineState *rs, int nb_args, const char *str); void netdev_add_completion(ReadLineState *rs, int nb_args, const char *str); void netdev_del_completion(ReadLineState *rs, int nb_args, const char *str); +void netfilter_add_completion(ReadLineState *rs, int nb_args, const char *str); +void netfilter_del_completion(ReadLineState *rs, int nb_args, const char *str); void ringbuf_write_completion(ReadLineState *rs, int nb_args, const char *str); void watchdog_action_completion(ReadLineState *rs, int nb_args, const char *str); diff --git a/include/net/filter.h b/include/net/filter.h index 7a858d8..f15d83d 100644 --- a/include/net/filter.h +++ b/include/net/filter.h @@ -53,5 +53,8 @@ NetFilterState *qemu_new_net_filter(NetFilterInfo *info, NetClientState *netdev, const char *name, int chain); +void qemu_del_net_filter(NetFilterState *nf); +void netfilter_add(QemuOpts *opts, Error **errp); +void qmp_netfilter_add(QDict *qdict, QObject **ret, Error **errp); #endif /* QEMU_NET_FILTER_H */ diff --git a/monitor.c b/monitor.c index fc32f12..58b43af 100644 --- a/monitor.c +++ b/monitor.c @@ -31,6 +31,7 @@ #include "hw/loader.h" #include "exec/gdbstub.h" #include "net/net.h" +#include "net/filter.h" #include "net/slirp.h" #include "sysemu/char.h" #include "ui/qemu-spice.h" @@ -4193,6 +4194,21 @@ void netdev_add_completion(ReadLineState *rs, int nb_args, const char *str) } } +void netfilter_add_completion(ReadLineState *rs, int nb_args, const char *str) +{ + size_t len; + int i; + + if (nb_args != 2) { + return; + } + len = strlen(str); + readline_set_completion_index(rs, len); + for (i = 0; NetFilterOptionsKind_lookup[i]; i++) { + add_completion_option(rs, str, NetFilterOptionsKind_lookup[i]); + } +} + void device_add_completion(ReadLineState *rs, int nb_args, const char *str) { GSList *list, *elt; @@ -4429,6 +4445,23 @@ void netdev_del_completion(ReadLineState *rs, int nb_args, const char *str) } } +void netfilter_del_completion(ReadLineState *rs, int nb_args, const char *str) +{ + int len; + QemuOpts *opts; + + if (nb_args != 2) { + return; + } + + len = strlen(str); + readline_set_completion_index(rs, len); + opts = qemu_opts_find(qemu_find_opts_err("netfilter", NULL), str); + if (opts) { + readline_add_completion(rs, str); + } +} + void watchdog_action_completion(ReadLineState *rs, int nb_args, const char *str) { int i; diff --git a/net/filter.c b/net/filter.c index cb23384..dcb1891 100644 --- a/net/filter.c +++ b/net/filter.c @@ -13,6 +13,7 @@ #include "qapi/opts-visitor.h" #include "qapi/dealloc-visitor.h" #include "qemu/config-file.h" +#include "qmp-commands.h" #include "net/filter.h" #include "net/net.h" @@ -41,7 +42,7 @@ NetFilterState *qemu_new_net_filter(NetFilterInfo *info, return nf; } -static inline void qemu_cleanup_net_filter(NetFilterState *nf) +static void qemu_cleanup_net_filter(NetFilterState *nf) { QTAILQ_REMOVE(&nf->netdev->filters, nf, next); QTAILQ_REMOVE(&net_filters, nf, global_list); @@ -54,6 +55,104 @@ static inline void qemu_cleanup_net_filter(NetFilterState *nf) g_free(nf); } +static int qemu_find_netfilters_by_name(const char *id, NetFilterState **nfs, + int max) +{ + NetFilterState *nf; + int ret = 0; + + QTAILQ_FOREACH(nf, &net_filters, global_list) { + if (!strcmp(nf->name, id)) { + if (ret < max) { + nfs[ret] = nf; + } + ret++; + } + } + + return ret; +} + +void qemu_del_net_filter(NetFilterState *nf) +{ + NetFilterState *nfs[MAX_QUEUE_NUM]; + int queues, i; + QemuOpts *opts; + + opts = qemu_opts_find(qemu_find_opts_err("netfilter", NULL), nf->name); + if (!opts) { + error_report("Options of '%s' can not be found", nf->name); + } + + queues = qemu_find_netfilters_by_name(nf->name, nfs, MAX_QUEUE_NUM); + assert(queues != 0); + + for (i = 0; i < queues; i++) { + qemu_cleanup_net_filter(nfs[i]); + } + + qemu_opts_del(opts); +} + +static NetFilterState *qemu_find_netfilter(const char *id) +{ + NetFilterState *nf; + + QTAILQ_FOREACH(nf, &net_filters, global_list) { + if (!strcmp(nf->name, id)) { + return nf; + } + } + + return NULL; +} + +static int net_init_filter(void *dummy, QemuOpts *opts, Error **errp); +void netfilter_add(QemuOpts *opts, Error **errp) +{ + net_init_filter(NULL, opts, errp); +} + +void qmp_netfilter_add(QDict *qdict, QObject **ret, Error **errp) +{ + Error *local_err = NULL; + QemuOptsList *opts_list; + QemuOpts *opts; + + opts_list = qemu_find_opts_err("netfilter", &local_err); + if (local_err) { + goto out; + } + + opts = qemu_opts_from_qdict(opts_list, qdict, &local_err); + if (local_err) { + goto out; + } + + netfilter_add(opts, &local_err); + if (local_err) { + qemu_opts_del(opts); + goto out; + } + +out: + error_propagate(errp, local_err); +} + +void qmp_netfilter_del(const char *id, Error **errp) +{ + NetFilterState *nf; + + nf = qemu_find_netfilter(id); + if (!nf) { + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, + "Filter '%s' not found", id); + return; + } + + qemu_del_net_filter(nf); +} + typedef int (NetFilterInit)(const NetFilterOptions *opts, const char *name, int chain, NetClientState *netdev, Error **errp); diff --git a/net/net.c b/net/net.c index d9b70cd..74f3592 100644 --- a/net/net.c +++ b/net/net.c @@ -28,6 +28,7 @@ #include "hub.h" #include "net/slirp.h" #include "net/eth.h" +#include "net/filter.h" #include "util.h" #include "monitor/monitor.h" @@ -385,6 +386,7 @@ void qemu_del_net_client(NetClientState *nc) { NetClientState *ncs[MAX_QUEUE_NUM]; int queues, i; + NetFilterState *nf, *next; assert(nc->info->type != NET_CLIENT_OPTIONS_KIND_NIC); @@ -396,6 +398,11 @@ void qemu_del_net_client(NetClientState *nc) MAX_QUEUE_NUM); assert(queues != 0); + /* qemu_del_net_filter() will handle the multiqueue case */ + QTAILQ_FOREACH_SAFE(nf, &nc->filters, next, next) { + qemu_del_net_filter(nf); + } + /* If there is a peer NIC, delete and cleanup client, but do not free. */ if (nc->peer && nc->peer->info->type == NET_CLIENT_OPTIONS_KIND_NIC) { NICState *nic = qemu_get_nic(nc->peer); 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 } + +## +# @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", + .args_type = "netfilter:O", + .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,
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(-)