Re: [Qemu-devel] [PATCH] New feature - RFC3931 L2TPv3 network transport using static Ethernet over L2TPv3 tunnels

[Eric Blake <eblake@redhat.com>]

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

On 03/05/2014 07:12 AM, anton.ivanov@kot-begemot.co.uk wrote:
> From: Anton Ivanov <antivano@cisco.com>
> 
> This transport allows qemu to communicate with host if host
> supports L2TPv3, communicate directly VM to VM (similar to
> current socket transport) and VM to other device - f.e. VM to
> a router.
> 
> Supported
>     * Raw IPv4, Raw IPv6, UDPv4 and UDPv6 as per RFC
>     * No cookie, 32 bit cookie or 64 bit cookie
>     * Counter (as per RFC)
>     * No counter (draft-mkonstan-keyed-ipv6-tunnel-00)
> Unsupported
>     * Workarounds for implementation with broken counter handling

Just an interface review (leaving the technical review to others more
knowledgeable on networking):

> +++ b/qapi-schema.json
> @@ -2940,6 +2940,57 @@
>      '*localaddr': 'str',
>      '*udp':       'str' } }
>  
> +# @NetdevL2TPv3Options
> +#
> +# Connect the VLAN to Ethernet over L2TPv3 Static tunnel
> +#
> +# @src :source address

s/ :/: /

> +#
> +# @dst :destination address

and again

> +#
> +# @srcport :#optional source port - mandatory for udp, optional for ip

and again (several times more)

udp is layer 3, ip is layer 2 - don't you mean "optional for tcp" as the
layer 3 counterpart of udp?

> +#
> +# @dstport :#optional destination port - mandatory for udp, optional for ip
> +#
> +# @ipv6 :#optional - force the use of ipv6 
> +#
> +# @udp :#optional - use the udp version of l2tpv3 encapsulation
> +#
> +# @cookie64:#optional - use 64 bit coookies

s/:/: /

> +#
> +# @counter :#optional have sequence counter
> +# 
> +# @txcookie :#optional 32 or 64 bit transmit cookie 
> +# 
> +# @rxcookie :#optional 32 or 64 bit receive cookie 
> +# 
> +# @txsession : 32 bit transmit session

s/ :/:/

> +# 
> +# @rxsession : 32 bit receive session - if not specified set to the same value as transmit

Long line, please wrap to keep within 80 columns

> +# 
> +# @optional : additional offset - allows the insertion of additional application-specific data before the packet payload

another long line

> +# 
> +#
> +# Since 1.2

At this point, you've pretty much missed the 2.0 feature freeze.  So the
earliest this can be is 2.1.  Specifying 1.2 is wrong either way.

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

TAB damage and trailing whitespace.  Please run your patch through
scripts/checkpatch.pl, and only use spaces in the .json file.

> +    'dst':	  'str', 
> +    '*srcport':	  'str', 
> +    '*dstport':	  'str', 
> +    '*ipv6':	  'bool', 
> +    '*udp':	  'bool', 
> +    '*cookie64':  'bool',

Indentation looks funky - either use a single space after : in all
cases, or align all types to the same column (I don't care which, as
long as it isn't the mismatch of random alignments that you currently have)

But looking better than the earlier version.  On your next submission,
be sure to use 'git send-email --subject-prefix=PATCHv2' to make it
obvious that you are sending a new version.

-- 
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 --]