Re: [Qemu-devel] Contribution - L2TPv3 transport

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3807 bytes --]

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.


> +++ 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).

> +#
> +# @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/

> +##
> +##
> +{ 'type': 'NetdevL2TPv3Options',
> +  'data': {
> +    '*fd':        'str',
> +    '*src':	  'str', 

You didn't list 'src' as optional above.

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.

> +
> +} }
> +
> +##
> +##
>  # @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',
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]