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
signature.asc
Description: OpenPGP digital signature
