qemu-devel
[Top][All Lists]
Advanced

[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: Yang Hongyang
Subject: Re: [Qemu-devel] [PATCH v8 03/11] netfilter: add netfilter_{add|del} commands
Date: Mon, 31 Aug 2015 09:36:34 +0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0

On 08/28/2015 07:37 PM, Markus Armbruster wrote:
Eric Blake <address@hidden> 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.


[...]
.


--
Thanks,
Yang.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]