Re: [Qemu-devel] Contribution - L2TPv3 transport

[Anton Ivanov (antivano)]

On 28/02/14 13:40, Eric Blake wrote:
> On 02/28/2014 01:28 AM, Anton Ivanov (antivano) wrote:
>> Hi all,
>>
>> On behalf of Cisco Systems I am authorized to contribute a new transport
>> to the network subsystem in qemu.
>>
>> Patch attached.
> Your patch is huge - it might be better to break it into smaller logical
> chunks to make it easier to review.  See this page for more hints:
> http://wiki.qemu.org/Contribute/SubmitAPatch
>
> Your patch is lacking a diffstat, and was sent as an attachment rather
> than inline.  This makes it harder to review.
>
> I'm going to focus my review on just the qapi interface for now.
>
>> +++ b/net/l2tpv3.c
>> @@ -0,0 +1,630 @@
>> +/*
>> + * QEMU System Emulator
>> + *
>> + * Copyright (c) 2003-2008 Fabrice Bellard
>> + * Copyright (c) 2012 Cisco Systems
> It's now 2014.


Fair point, will fix.
>
>
>> +++ b/qapi-schema.json
>> @@ -2941,6 +2941,45 @@
>>      '*udp':       'str' } }
>>  
>>  ##
>> +# @NetdevL2TPv3Options
>> +#
>> +# Connect the VLAN to Ethernet over L2TPv3 Static tunnel
>> +#
>> +# @fd: #optional file descriptor of an already opened socket
> Other commands name this parameter 'fdname' (see 'add_client'); it takes
> an fd named via the 'getfd' command.  However, this style is old, and
> new code should instead prefer file names rather than fd names.  That
> is, we want the newer style of using 'add-fd', then referring to the
> file by name (/dev/fdset/nnn), and not the older style of 'getfd'; using
> a file name also gives you the flexibility of using actual Unix socket
> files stored in the file system.
>
>> +#
>> +# @src: source address
>> +#
>> +# @dst: #optional destination address
>> +#
>> +# @mode: bitmask mode for the tunnel (see l2tpv3.h for actual definition)
> Ewww.  This is is gross.  This API should be self-documented here,
> without making the end-reader chase down a source file.  And passing raw
> numbers for bit ids is not user-friendly.  Better would be making this
> parameter be an enum (if you can describe all modes via a single name,
> as in 'mode':'cookie') or a series of bool arguments (perhaps optional
> with sane defaults, as in 'udp':true,'cookie':false).

I discussed this with Paolo, a fixed version compliant to coding style
guidelines and with a user friendly arg list will be made available in a
week or two.
>
>> +#
>> +# @txcookie:#optional 32 or 64 bit tx cookie for the tunnel (set mode for 
>> actual type selection)
>> +#
>> +# @rxcookie:#optional 32 or 64 bit rx cookie for the tunnel (set mode for 
>> actual type selection)
>> +#
>> +# @txsession: tx 32 bit session
>> +#
>> +# @rxsession: rx 32 bit session
>> +#
>> +#
>> +# Since 1.0
> s/1.0/2.0/

OK - just to clarify, which version is this referring to - qemu, api, etc?

>
>> +##
>> +##
>> +{ 'type': 'NetdevL2TPv3Options',
>> +  'data': {
>> +    '*fd':        'str',
>> +    '*src':   'str', 
> You didn't list 'src' as optional above.

That is intended. It is not optional for the ip command in Linux either.
For L2TPv3 tunnels you have to specify the source address.

>
> Trailing whitespace - run your submission through checkpatch.pl.
>
>> +    '*dst':   'str', 
>> +    '*mode':          'str', 
> As mentioned above, 'mode' should not be a string.
>
>> +    '*txcookie':  'str',
>> +    '*rxcookie':  'str',
>> +    '*txsession': 'str',
>> +    '*rxsession': 'str'
> These should probably be 'int', not 'str'.  If you have to hand-parse a
> string into a numeric value, you encoded the QMP wrong.

These by spec are either 32 bit or 64 bit integers. I will fix them
accordingly.

>
>> +
>> +} }
>> +
>> +##
>> +##
>>  # @NetdevVdeOptions
>>  #
>>  # Connect the VLAN to a vde switch running on the host.
>> @@ -3021,6 +3060,7 @@
>>      'nic':      'NetLegacyNicOptions',
>>      'user':     'NetdevUserOptions',
>>      'tap':      'NetdevTapOptions',
>> +    'l2tpv3':   'NetdevL2TPv3Options',
> Please add documentation that the l2tpv3 arm of this union was added in
> 2.0 (yes, I know we haven't been very good at documenting when unions
> were extended, but it can't hurt to do a better job going forward).
>
>>      'socket':   'NetdevSocketOptions',
>>      'vde':      'NetdevVdeOptions',
>>      'dump':     'NetdevDumpOptions',
>>