Eric Blake <ebl...@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.
[...]
