Re: [Qemu-devel] Contribution - L2TPv3 transport

[Anton Ivanov (antivano)]

On 04/03/14 15:53, Eric Blake wrote:
> On 03/04/2014 08:22 AM, Anton Ivanov (antivano) wrote:
>> Apologies, missed to diff the json definitions.
>>
>> Attached.
>>
> Missing a commit message and a Signed-off-by line, so it can't be
> applied as-is.  Also, we prefer inline patches (as sent by 'git
> send-email') over attached patches; I suggest using 'git send-email' to
> first send the patches to yourself to make sure your settings are correct.

Will do. Can you review the actual transport patch please.

>
>
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index 83fa485..56eac6d 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -2940,6 +2940,62 @@
>>       '*localaddr': 'str',
>>       '*udp':       'str' } }
>>   
>> +# @NetdevL2TPv3Options
>> +#
>> +# Connect the VLAN to Ethernet over L2TPv3 Static tunnel
>> +#
>> +# @ipv6: #bool, use ipv6
>> +#
> This should be:
>
> # @ipv6: #optional true to use ipv6, default false
>
> That is, we have a marking '#optional', but do not have a marking
> '#bool' (you can read the actual definition below to learn that 'ipv6'
> is a bool type).
>
> Also, it makes it easier if you document options in the same order as
> they appear in the struct below (there, you have 'fd' first).
>
>> +# @udp: #bool use the udp version of the L2TPv3 encapsulation
> Again, #optional, not #bool, and mention the default value
>
>> +#
>> +# @cookie64 : #use 64 bit cookies
> #optional
>
>> +#
>> +# @offset : #extra offset
> #optional
>
>> +#
>> +# @counter : #have sequence counter
> #optional
>
>> +#
>> +# @fd: #optional file descriptor of an already opened socket
> This doesn't seem to take into account my earlier comments - is the goal
> to allow opening both files from the file system and the magic string
> '/dev/fdset/...' supported by use of 'add-fd'?

Yes, but not yet supported in this version. I will cut it out for now.

>
>> +#
>> +# @src: #source address
> Should be:
>
> # @src: source address
>
> The only use of # inside the docs has been for the tag '#optional'
>
>> +#
>> +# @srcport: #source port - mandatory for udp, optional for ip
>> +#
>> +# @dst: #destination address
>> +#
>> +# @dstport: #destination port - mandatory for udp, optional for ip
>> +#
>> +# @txcookie: #optional 32 or 64 bit tx cookie for the tunnel
>> +#
>> +# @rxcookie: #optional 32 or 64 bit rx cookie for the tunnel
>> +#
>> +# @txsession: #tx 32 bit session
>> +#
>> +# @rxsession: #rx 32 bit session - if unset value for txsession is used
> Should be:
>
> # @rxsession: #optional rx 32 bit session, defaults to @txsession
>
>> +#
>> +#
>> +# Since 1.2
> Should be:
>
> Since 2.0
>
>> +##
>> +##
>> +{ 'type': 'NetdevL2TPv3Options',
>> +  'data': {
>> +    '*fd':        'str',
>> +    'src':    'str',
>> +    'dst':    'str',
>> +    '*srcport':       'str',
>> +    '*dstport':       'str',
>> +    '*ipv6':          'bool',
>> +    '*udp':   'bool',
>> +    '*cookie64':  'bool',
>> +    '*counter':   'bool',
>> +    '*txcookie':  'uint64',
>> +    '*rxcookie':  'uint64',
>> +    'txsession': 'uint32',
>> +    '*rxsession': 'uint32',
>> +    '*offset':  'uint32'
>> +
> Why the blank line?
>> +} }
>> +
>> +##
>>   ##
>>   # @NetdevVdeOptions
>>   #
>> @@ -3014,13 +3070,16 @@
>>   # A discriminated record of network device traits.
>>   #
>>   # Since 1.2
>> -##
>> +#
>> +# Added in 2.0 - l2tpv3
>> +#
>>   { 'union': 'NetClientOptions',
>>     'data': {
>>       'none':     'NetdevNoneOptions',
>>       'nic':      'NetLegacyNicOptions',
>>       'user':     'NetdevUserOptions',
>>       'tap':      'NetdevTapOptions',
>> +    'l2tpv3':   'NetdevL2TPv3Options',
>>       'socket':   'NetdevSocketOptions',
>>       'vde':      'NetdevVdeOptions',
>>       'dump':     'NetdevDumpOptions',
>>