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

[Yang Hongyang <yanghy@cn.fujitsu.com>]

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.

>
> [...]
> .
>

-- 
Thanks,
Yang.