[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v8 03/11] netfilter: add netfilter_{add|del} com
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v8 03/11] netfilter: add netfilter_{add|del} commands |
Date: |
Wed, 26 Aug 2015 09:37:23 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0 |
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 +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
- Re: [Qemu-devel] [PATCH v8 01/11] net: add a new object netfilter, (continued)
[Qemu-devel] [PATCH v8 04/11] netfilter: hook packets before net queue send, Yang Hongyang, 2015/08/26
[Qemu-devel] [PATCH v8 02/11] init/cleanup of netfilter object, Yang Hongyang, 2015/08/26
[Qemu-devel] [PATCH v8 03/11] netfilter: add netfilter_{add|del} commands, Yang Hongyang, 2015/08/26
- Re: [Qemu-devel] [PATCH v8 03/11] netfilter: add netfilter_{add|del} commands, Markus Armbruster, 2015/08/26
- Re: [Qemu-devel] [PATCH v8 03/11] netfilter: add netfilter_{add|del} commands,
Eric Blake <=
- Re: [Qemu-devel] [PATCH v8 03/11] netfilter: add netfilter_{add|del} commands, Markus Armbruster, 2015/08/28
- Re: [Qemu-devel] [PATCH v8 03/11] netfilter: add netfilter_{add|del} commands, Yang Hongyang, 2015/08/30
- Re: [Qemu-devel] [PATCH v8 03/11] netfilter: add netfilter_{add|del} commands, Markus Armbruster, 2015/08/31
- Re: [Qemu-devel] [PATCH v8 03/11] netfilter: add netfilter_{add|del} commands, Yang Hongyang, 2015/08/31
- Re: [Qemu-devel] [PATCH v8 03/11] netfilter: add netfilter_{add|del} commands, Eric Blake, 2015/08/31
- Re: [Qemu-devel] [PATCH v8 03/11] netfilter: add netfilter_{add|del} commands, Yang Hongyang, 2015/08/31
[Qemu-devel] [PATCH v8 05/11] move out net queue structs define, Yang Hongyang, 2015/08/26
[Qemu-devel] [PATCH v8 06/11] netfilter: add an API to pass the packet to next filter, Yang Hongyang, 2015/08/26