On Fri, Feb 28, 2014 at 08:28:11AM +0000, Anton Ivanov (antivano) wrote:
I didn't check all of Paolo and Eric's comments. Feel free to skip
comments that have already been made.
> diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
> index 45588d7..865f1c2 100644
> --- a/include/qemu/sockets.h
> +++ b/include/qemu/sockets.h
> @@ -79,6 +79,7 @@ int socket_dgram(SocketAddress *remote, SocketAddress
> *local, Error **errp);
>
> /* Old, ipv4 only bits. Don't use for new code. */
> int parse_host_port(struct sockaddr_in *saddr, const char *str);
> +int parse_host_port6(struct sockaddr_in6 *saddr, const char *str);
> int socket_init(void);
>
> #endif /* QEMU_SOCKET_H */
Paolo posted ipv4/ipv6 follow-ups and suggestions, so maybe you won't
need this in the next version.
But right off the bat, adding a utility function to a common source file
is a good indicator that this should be a separate patch. Splitting the
code up into multiple patches gives you a chance to explain the
rationale for one change at a time, aids bisectability, and makes code
review easier.
> diff --git a/net/Makefile.objs b/net/Makefile.objs
> index 4854a14..364c785 100644
> --- a/net/Makefile.objs
> +++ b/net/Makefile.objs
> @@ -2,6 +2,7 @@ common-obj-y = net.o queue.o checksum.o util.o hub.o
> common-obj-y += socket.o
> common-obj-y += dump.o
> common-obj-y += eth.o
> +common-obj-y += l2tpv3.o
> common-obj-$(CONFIG_POSIX) += tap.o
> common-obj-$(CONFIG_LINUX) += tap-linux.o
> common-obj-$(CONFIG_WIN32) += tap-win32.o
> diff --git a/net/clients.h b/net/clients.h
> index 7793294..ea2a831 100644
> --- a/net/clients.h
> +++ b/net/clients.h
> @@ -47,6 +47,11 @@ int net_init_tap(const NetClientOptions *opts, const char
> *name,
> int net_init_bridge(const NetClientOptions *opts, const char *name,
> NetClientState *peer);
>
> +int net_init_raw(const NetClientOptions *opts, const char *name,
> + NetClientState *peer);
> +
Think you already noticed, but the 'raw' net client code slipped into
this patch. Please split it into a separate patch since it's not
directly part of the L2TPv3 code changes.
I suggest making the 'raw' net client a completely separate patch
series. Post a separate email thread so it can be reviewed and merged
independently of L2TPv3.
> +int net_init_l2tpv3(const NetClientOptions *opts, const char *name,
> + NetClientState *peer);
> #ifdef CONFIG_VDE
> int net_init_vde(const NetClientOptions *opts, const char *name,
> NetClientState *peer);
> diff --git a/net/l2tpv3.c b/net/l2tpv3.c
> new file mode 100644
> index 0000000..4b5d8ee
> --- /dev/null
> +++ b/net/l2tpv3.c
> @@ -0,0 +1,630 @@
> +/*
> + * QEMU System Emulator
> + *
> + * Copyright (c) 2003-2008 Fabrice Bellard
> + * Copyright (c) 2012 Cisco Systems
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> copy
> + * of this software and associated documentation files (the "Software"), to
> deal
> + * in the Software without restriction, including without limitation the
> rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include "net/l2tpv3.h"
> +#include "config-host.h"
> +#include "config-host.h"
> +#include "net/net.h"
> +#include "clients.h"
> +#include "monitor/monitor.h"
> +#include "qemu-common.h"
> +#include "qemu/error-report.h"
> +#include "qemu/option.h"
> +#include "qemu/sockets.h"
> +#include "qemu/iov.h"
> +#include "qemu/main-loop.h"
> +#include <linux/ip.h>
Please #include <system/headers.h> before #include "user/headers.h" just
to make sure our user headers don't interfere with system headers. For
example by defining MIN()/MAX() or other macros.
> +
> +
> +
> +
> +#define PAGE_SIZE 4096
If you really *need* the page size, please use sysconf(_SC_PAGESIZE).
If you just need a big buffer size, please rename this to L2TPV3_BUFSIZE
or similar to avoid linking the name to page size.
> +#define MAX_L2TPV3_IOVCNT 64
> +
> +typedef struct NetL2TPV3State {
> + NetClientState nc;
> + int fd;
> + int state; /* 0 = getting length, 1 = getting data */
> + unsigned int index;
> + unsigned int packet_len;
state, index, and packet_len are unused. Please remove.
> + /*
> + these are used for xmit - that happens packet a time
> + and for first sign of life packet (easier to parse that once)
> +
> + */
> + uint8_t * header_buf;
> + uint8_t * buf;
> + struct iovec * vec;
> + /*
> + these are used for receive - try to "eat" up to 32 packets at a time
It's best to avoid referring explicitly to 32 here. It can easily get
out-of-sync with the code.
> + */
> + struct mmsghdr * msgvec;
> + /*
> + contains inet host and port destination if
> + connectionless (SOCK_DGRAM). Will be NULL if
> + in listen mode waiting for first connect
> +
> + */
> + struct sockaddr_storage * dgram_dst;
> + uint64_t rx_cookie;
> + uint64_t tx_cookie;
> + uint32_t rx_session;
> + uint32_t tx_session;
> + uint32_t header_size;
> + uint32_t counter;
> + uint32_t dst_size;
> + uint32_t new_mode;
Why is it called "new" mode?
> +
> + /* offsets */
> +
> + uint32_t offset; /* main offset == header offset */
I suggest using a descriptive name. header_offset?
> + uint32_t cookie_offset;
> + uint32_t counter_offset;
> + uint32_t session_offset;
> +
> +} NetL2TPV3State;
> +
> +typedef struct NetL2TPV3ListenState {
> + NetClientState nc;
> + char *model;
> + char *name;
> + int fd;
> +} NetL2TPV3ListenState;
Unused, please remove.
> +static ssize_t net_l2tpv3_receive_dgram(NetClientState *nc, const uint8_t
> *buf, size_t size)
> +{
> + NetL2TPV3State *s = DO_UPCAST(NetL2TPV3State, nc, nc);
> +
> + struct iovec * vec;
> +
> + struct msghdr message;
> + l2tpv3_form_header(s);
> + vec = s->vec;
> + vec->iov_base = s->header_buf;
> + vec->iov_len = s->offset;
> + vec++;
> + vec->iov_base = (void *) buf;
> + vec->iov_len = size;
> +
> + if (s->dgram_dst) {
> + message.msg_name = s->dgram_dst;
> + message.msg_namelen = s->dst_size;
> + message.msg_iov = (struct iovec *) s->vec;
> + message.msg_iovlen = 2;
> + message.msg_control = NULL;
> + message.msg_controllen = 0;
> + message.msg_flags = 0;
> + size = sendmsg(s->fd, &message, 0) - s->offset;
Explicit sendmesg() -1 return handling would be nice. Also, please use
ssize_t (signed) for manipulating the return value instead of size_t
(unsigned).
What happens if the error was EINTR we can retry. If the error was
EAGAIN we should return 0 and wait for the fd to become writable. When
it becomes writable we should flush the queue (see
qemu_net_queue_flush()). Other net clients already do this, so there is
example code to look at.
> +static void net_l2tpv3_send(void *opaque)
> +{
> + NetL2TPV3State *s = opaque;
> +
> + int i, count, offset;
> +
> + struct mmsghdr * msgvec;
> + struct iovec * vec;
> +
> +
> + msgvec = s->msgvec;
> + offset = s->offset;
> + if ((!(s->new_mode & NEW_MODE_IP_VERSION)) && (!(s->new_mode &
> NEW_MODE_UDP))){
> + offset += sizeof(struct iphdr);
> + }
> + count = recvmmsg(s->fd, msgvec, MAX_L2TPV3_IOVCNT/2, MSG_DONTWAIT, NULL);
> + for (i=0;i<count;i++) {
> + if (msgvec->msg_len > 0) {
> + vec = msgvec->msg_hdr.msg_iov;
> + if (l2tpv3_verify_header(s, vec->iov_base) == 0) {
Header verification should check the size of the packet too. It must be
large enough for the L2TPv3 header, otherwise we'd access stale data
that happened to be in vec->iov_base...
> + //vec->iov_len = offset; /* reset size just in case*/
> + vec++;
> + qemu_send_packet(&s->nc, vec->iov_base, msgvec->msg_len -
> offset);
...and if the header check happened to pass, the msgvec->msg_len -
offset argument value will be negative (int)!
> + } else {
> + fprintf(stderr, "l2tpv3 header verification failed\n");
> + vec->iov_len = offset;
> + vec++;
> + }
> + //vec->iov_len = PAGE_SIZE; /* reset for next read */
I think it *is* necessary to reset ->iov_len for both msgvec iovecs.
> +static int net_l2tpv3_init(NetClientState *peer,
> + const char *name,
> + const char *src,
> + const char *dst,
> + const char *new_mode,
> + const char *tx_cookie,
> + const char *rx_cookie,
> + const char *tx_session,
> + const char *rx_session
> +)
> +{
> + NetL2TPV3State *s;
> + NetClientState *nc;
> +
> + int fd, i;
> +
> + int sock_family, sock_type, sock_proto;
> + unsigned int temp;
> +
> + struct mmsghdr * msgvec;
> + struct iovec * iov;
> +
> +
> + struct sockaddr_storage LocalSock;
> +
> +
> + fprintf(stderr, "l2tpv3 user init mode %s\n", new_mode);
If you want observability, please use trace events: docs/tracing.txt.
You can define trace events in ./trace-events and they can be used with
DTrace/SystemTap/LTTng/ftrace/stderr.
It's often useful to define trace events for important state transitions
in the code (e.g. creation, starting a connection, completing a
connection, sending a packet, receiving a packet, disconnecting, etc).
> + memset(&LocalSock, 0 , sizeof(LocalSock));
> +
> + nc = qemu_new_net_client(&net_l2tpv3_info, peer, "l2tpv3", name);
> +
> + s = DO_UPCAST(NetL2TPV3State, nc, nc);
> +
> + s->header_buf = g_malloc(256);
Can you use C structs and unions instead of choosing an arbitrary
256-byte size and calculating offsets at runtime?
typedef struct {
uint16_t flags;
uint16_t reserved;
uint32_t session_id;
union {
uint32_t cookie32;
uint64_t cookie64;
};
} QEMU_PACKED L2TPv3SessionHeader;
Or perhaps something like:
typedef union {
L2TPv3SessionHeader32 session32;
L2TPv3SessionHeader64 session64;
L2TPv3DataHeader data;
} L2TPv3SessionHeader;
(I haven't checked which approach is simplest but it would make the code
clearer and avoid byte manipulation.)
> + if (s->header_buf == NULL) {
> + fprintf(stderr, "Failed to allocate header buffer \n");
> + return -1;
> + }
g_malloc() never fails, it calls abort(3) on out-of-memory. The same
applies to other g_malloc() calls in this patch.
> + s->buf = g_malloc(PAGE_SIZE);
> +
> + if (s->buf == NULL) {
> + fprintf(stderr, "Failed to allocate packet buffer \n");
> + return -1;
> + }
This buffer isn't used?
> + s->vec = g_malloc(sizeof(struct iovec) * MAX_L2TPV3_IOVCNT);;
Slightly more concise way to say the same thing:
s->vec = g_new(struct iovec, MAX_L2TPV3_IOVCNT);
> + if (dst && (strlen(dst) > 0 )) {
> + /* we now allocate it only if it we are not "listening" */
> + s->dgram_dst = malloc(sizeof(struct sockaddr_storage));
Please use g_malloc()/g_new() instead of malloc(3).
> + } else {
> + sock_type = SOCK_RAW;
> + sock_proto = 0x73;
Please use the IPPROTO_L2TP constant instead of a magic number.
> + for (i=0;i<MAX_L2TPV3_IOVCNT/2;i++) {
> + /* we need to allocate these only for the first time and first buffer */
> + msgvec->msg_hdr.msg_name = NULL;
> + msgvec->msg_hdr.msg_namelen = 0;
> + iov = g_malloc(sizeof(struct iovec) * 2);
> + if (iov == NULL) {
> + fprintf(stderr, "Failed to allocate receive packet vec \n");
> + return -1;
> + }
> + msgvec->msg_hdr.msg_iov = iov;
> + if ((!(s->new_mode & NEW_MODE_IP_VERSION)) && (!(s->new_mode &
> NEW_MODE_UDP))){
> + iov->iov_base = g_malloc(s->offset + sizeof(struct iphdr)); /* fix
> for ipv4 raw */;
Is there a way to disable the IP header included in received packets? I
haven't looked into how IP_HDRINCL works...
> + } else {
> + iov->iov_base = g_malloc(s->offset);
> + }
> +
> + if (iov->iov_base == NULL) {
> + fprintf(stderr, "Failed to allocate receive packet vec \n");
> + return -1;
> + }
> + if ((!(s->new_mode & NEW_MODE_IP_VERSION)) && (!(s->new_mode &
> NEW_MODE_UDP))){
> + iov->iov_len = s->offset + sizeof (struct iphdr);
> + } else {
> + iov->iov_len = s->offset;
> + }
I suggest setting iov_len first, and then allocating iov_base using that
size. This way you don't duplicate the IPv4 raw check.
> + qemu_set_fd_handler(s->fd, net_l2tpv3_send, NULL, s);
> +
> + if (!s) {
> + fprintf (stderr, "l2tpv3_open : failed to set fd handler\n");
> + return -1;
> + }
> +
> + /* this needs fixing too */
Please fix it :)
> +
> + snprintf(s->nc.info_str, sizeof(s->nc.info_str),
> + "l2tpv3: connected");
> + return 0;
> +
> +}
> +
> +int net_init_l2tpv3(const NetClientOptions *opts,
> + const char *name,
> + NetClientState *peer) {
> +
> +
> + const NetdevL2TPv3Options * l2tpv3;
> + assert(opts->kind == NET_CLIENT_OPTIONS_KIND_L2TPV3);
> + l2tpv3 = opts->l2tpv3;
> +
> +
> + if (!(l2tpv3->has_src && l2tpv3->has_mode && l2tpv3->has_txsession &&
> l2tpv3->has_rxsession)) {
> + error_report("src=, dst=, txsession=, rxsession= and mode= are
> required for l2tpv3");
> + return -1;
> + }
If they are required then I'm not sure why they were declared optional
in qapi-schema.json. Drop the '*' in the schema and these fields become
mandatory.
> +
> +
> + /* this needs cleaning up for new API */
> +
> + if (net_l2tpv3_init(
> + peer,
> + name,
> + l2tpv3->src,
> + l2tpv3->dst,
> + l2tpv3->mode,
> + l2tpv3->txcookie,
> + l2tpv3->rxcookie,
> + l2tpv3->txsession,
> + l2tpv3->rxsession
> +
> + ) == -1) {
> + return -1;
> + }
> + return 0;
> +}
> diff --git a/net/l2tpv3.h b/net/l2tpv3.h
> new file mode 100644
> index 0000000..822e0b0
> --- /dev/null
> +++ b/net/l2tpv3.h
> @@ -0,0 +1,65 @@
A header file is not necessary. These constants are internal to the
L2TPv3 implementation, please put them in the .c file.
> +/*
> + * QEMU System Emulator
> + *
> + * Copyright (c) 2003-2008 Fabrice Bellard
> + * Copyright (c) 2012 Cisco Systems
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> copy
> + * of this software and associated documentation files (the "Software"), to
> deal
> + * in the Software without restriction, including without limitation the
> rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include "qemu-common.h"
> +
> +#ifndef QEMU_NET_L2TPV3_H
> +#define QEMU_NET_L2TPV3_H
> +
> +#define L2TPV3_BUFSIZE 2048
Hey, I see you did define an L2TPV3_BUFSIZE at one point in time after
all ;-). But it's unused.
> +
> +#define NEW_MODE_IP_VERSION 1 /* on for v6, off for v4 */
> +#define NEW_MODE_UDP 2 /* on for udp, off for raw ip */
> +#define NEW_MODE_COOKIE 4 /* cookie present */
> +#define NEW_MODE_COOKIE_SIZE 8 /* on for 64 bit */
> +#define NEW_MODE_NO_COUNTER 16 /* DT - no counter */
> +
> +/* legacy modes */
> +
> +/* mode 0 */
> +
> +#define LEGACY_UDP6_64_NO_COUNTER (NEW_MODE_IP_VERSION + NEW_MODE_UDP +
> NEW_MODE_COOKIE + NEW_MODE_COOKIE_SIZE + NEW_MODE_NO_COUNTER)
> +
> +#define LEGACY_MODE0 LEGACY_UDP6_64_NO_COUNTER
> +
> +/* mode 1 */
> +
> +#define LEGACY_IP6_64_NO_COUNTER (NEW_MODE_IP_VERSION + NEW_MODE_COOKIE +
> NEW_MODE_COOKIE_SIZE + NEW_MODE_NO_COUNTER)
> +
> +#define LEGACY_MODE1 LEGACY_IP6_64_NO_COUNTER
> +
> +/* mode 2 */
> +
> +#define LEGACY_UDP4_64_COUNTER (NEW_MODE_COOKIE + NEW_MODE_UDP +
> NEW_MODE_COOKIE_SIZE )
> +
> +#define LEGACY_MODE2 LEGACY_UDP4_64_COUNTER
The legacy mode constants are not used, please remove.
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 83fa485..192d19c 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2941,6 +2941,45 @@
> '*udp': 'str' } }
>
> ##
> +# @NetdevL2TPv3Options
> +#
> +# Connect the VLAN to Ethernet over L2TPv3 Static tunnel
The term "VLAN" in QEMU is outdated. QEMU now uses a peer-to-peer
topology of NetClientState instances.
I suggest "Connect to an Ethernet-over-L2TPv3 static tunnel"
> +#
> +# @fd: #optional file descriptor of an already opened socket
This argument isn't used.
> +#
> +# @src: source address
> +#
> +# @dst: #optional destination address
> +#
> +# @mode: bitmask mode for the tunnel (see l2tpv3.h for actual definition)
> +#
> +# @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
> +##
> +##
> +{ 'type': 'NetdevL2TPv3Options',
> + 'data': {
> + '*fd': 'str',
> + '*src': 'str',
> + '*dst': 'str',
> + '*mode': 'str',
> + '*txcookie': 'str',
> + '*rxcookie': 'str',
> + '*txsession': 'str',
> + '*rxsession': 'str'
> +
> +} }
I guess you already got plenty of feedback from Eric and Paolo on how to
type these parameters and avoid the bitmask :-).
