pespin submitted this change.

View Change

Refactor tun_t fields and alloc APIs

The previous state of code made allocation code more complex for no good
reason; use usual alloc() type function instead, splitting between the 2
types of tun_t implementations available to make it easier to
understand.

Move the several tun-iface specific mode fields to a substruct to make
it clear those are only really containing useful information when the
tun_t is created in tun-iface mode.

Change-Id: Ic71f91c62cd5bd48c6d35534eaac2091e4c69735
---
M ggsn/ggsn.c
M lib/tun.c
M lib/tun.h
M sgsnemu/sgsnemu.c
4 files changed, 109 insertions(+), 84 deletions(-)

diff --git a/ggsn/ggsn.c b/ggsn/ggsn.c
index 062e542..329b15f 100644
--- a/ggsn/ggsn.c
+++ b/ggsn/ggsn.c
@@ -226,7 +226,8 @@
 	switch (apn->cfg.gtpu_mode) {
 	case APN_GTPU_MODE_TUN:
 		LOGPAPN(LOGL_INFO, apn, "Opening TUN device %s\n", apn->tun.cfg.dev_name);
-		if (tun_new(&apn->tun.tun, apn->tun.cfg.dev_name, false, -1, -1)) {
+		apn->tun.tun = tun_alloc_tundev(apn->tun.cfg.dev_name);
+		if (!apn->tun.tun) {
 			LOGPAPN(LOGL_ERROR, apn, "Failed to configure tun device\n");
 			return -1;
 		}
@@ -246,7 +247,8 @@
 			return 0;
 		}
 		/* use GTP kernel module for data packet encapsulation */
-		if (tun_new(&apn->tun.tun, apn->tun.cfg.dev_name, true, gsn->fd0, gsn->fd1u)) {
+		apn->tun.tun = tun_alloc_gtpdev(apn->tun.cfg.dev_name, gsn->fd0, gsn->fd1u);
+		if (!apn->tun.tun) {
 			LOGPAPN(LOGL_ERROR, apn, "Failed to configure Kernel GTP device\n");
 			return -1;
 		}
diff --git a/lib/tun.c b/lib/tun.c
index 3cce3a5..89fe292 100644
--- a/lib/tun.c
+++ b/lib/tun.c
@@ -93,102 +93,118 @@
 	return rc;
 }
 
-int tun_new(struct tun_t **tun, const char *dev_name, bool use_kernel, int fd0, int fd1u)
+static struct tun_t *tun_alloc_common(const char *devname)
 {
-	struct tun_t *t;
+	struct tun_t *tun;
+
+	tun = talloc_zero(NULL, struct tun_t);
+	if (!tun) {
+		LOGP(DTUN, LOGL_ERROR, "tun_alloc_common() failed\n");
+		return NULL;
+	}
+
+	tun->cb_ind = NULL;
+	tun->addrs = 0;
+	tun->tundev.fd = -1;
+
+	osmo_strlcpy(tun->devname, devname, IFNAMSIZ);
+	tun->devname[IFNAMSIZ - 1] = 0;
+
+	return tun;
+}
+
+struct tun_t *tun_alloc_tundev(const char *devname)
+{
+	struct tun_t *tun;
 	int rc;
 
-	t = talloc_zero(NULL, struct tun_t);
-	if (!t) {
-		SYS_ERR(DTUN, LOGL_ERROR, errno, "talloc_zero() failed");
-		return EOF;
+	tun = tun_alloc_common(devname);
+	if (!tun)
+		return NULL;
+
+	tun->tundev.tundev = osmo_tundev_alloc(tun, tun->devname);
+	if (!tun->tundev.tundev)
+		goto err_free;
+	osmo_tundev_set_priv_data(tun->tundev.tundev, tun);
+	osmo_tundev_set_data_ind_cb(tun->tundev.tundev, tun_tundev_data_ind_cb);
+	rc = osmo_tundev_set_dev_name(tun->tundev.tundev, tun->devname);
+	if (rc < 0)
+		goto err_free_tundev;
+
+	/* Open the actual tun device */
+	rc = osmo_tundev_open(tun->tundev.tundev);
+	if (rc < 0)
+		goto err_free_tundev;
+	tun->tundev.fd = osmo_tundev_get_fd(tun->tundev.tundev);
+	tun->netdev = osmo_tundev_get_netdev(tun->tundev.tundev);
+
+	/* Disable checksums */
+	if (ioctl(tun->tundev.fd, TUNSETNOCSUM, 1) < 0) {
+		SYS_ERR(DTUN, LOGL_NOTICE, errno, "could not disable checksum on %s", tun->devname);
 	}
-	*tun = t;
 
-	t->cb_ind = NULL;
-	t->addrs = 0;
-	t->fd = -1;
+	LOGP(DTUN, LOGL_NOTICE, "tun %s configured\n", tun->devname);
+	return tun;
 
-	if (!use_kernel) {
-		osmo_strlcpy(t->devname, dev_name, IFNAMSIZ);
-		t->devname[IFNAMSIZ - 1] = 0;
-
-		t->tundev = osmo_tundev_alloc(t, dev_name);
-		if (!t->tundev)
-			goto err_free;
-		osmo_tundev_set_priv_data(t->tundev, t);
-		osmo_tundev_set_data_ind_cb(t->tundev, tun_tundev_data_ind_cb);
-		rc = osmo_tundev_set_dev_name(t->tundev, dev_name);
-		if (rc < 0)
-			goto err_free_tundev;
-
-		/* Open the actual tun device */
-		rc = osmo_tundev_open(t->tundev);
-		if (rc < 0)
-			goto err_free;
-		t->fd = osmo_tundev_get_fd(t->tundev);
-		t->netdev = osmo_tundev_get_netdev(t->tundev);
-
-		/* Disable checksums */
-		if (ioctl(t->fd, TUNSETNOCSUM, 1) < 0) {
-			SYS_ERR(DTUN, LOGL_NOTICE, errno, "could not disable checksum on %s", t->devname);
-		}
-
-		LOGP(DTUN, LOGL_NOTICE, "tun %s configured\n", t->devname);
-		return 0;
 err_free_tundev:
-	osmo_tundev_free(t->tundev);
+	osmo_tundev_free(tun->tundev.tundev);
 err_free:
-	talloc_free(t);
-	*tun = NULL;
-	return -1;
+	talloc_free(tun);
+	return NULL;
+}
 
-	} else {
-		osmo_strlcpy(t->devname, dev_name, IFNAMSIZ);
-		t->devname[IFNAMSIZ - 1] = 0;
+struct tun_t *tun_alloc_gtpdev(const char *devname, int fd0, int fd1u)
+{
+	struct tun_t *tun;
+	int rc;
 
-		if (gtp_kernel_create(-1, dev_name, fd0, fd1u) < 0) {
-			LOGP(DTUN, LOGL_ERROR, "cannot create GTP tunnel device: %s\n",
-				strerror(errno));
-			return -1;
-		}
-		t->netdev = osmo_netdev_alloc(t, dev_name);
-		if (!t->netdev)
-			goto err_kernel_create;
-		rc = osmo_netdev_set_ifindex(t->netdev, if_nametoindex(dev_name));
-		if (rc < 0)
-			goto err_netdev_free;
-		rc = osmo_netdev_register(t->netdev);
-		if (rc < 0)
-			goto err_netdev_free;
-		LOGP(DTUN, LOGL_NOTICE, "GTP kernel configured\n");
-		return 0;
-err_netdev_free:
-	osmo_netdev_free(t->netdev);
-err_kernel_create:
-	gtp_kernel_stop(t->devname);
-	return -1;
+	tun = tun_alloc_common(devname);
+	if (!tun)
+		return NULL;
+
+	if (gtp_kernel_create(-1, tun->devname, fd0, fd1u) < 0) {
+		LOGP(DTUN, LOGL_ERROR, "cannot create GTP tunnel device: %s\n",
+			strerror(errno));
+		goto err_free;
 	}
+	tun->netdev = osmo_netdev_alloc(tun, tun->devname);
+	if (!tun->netdev)
+		goto err_kernel_create;
+	rc = osmo_netdev_set_ifindex(tun->netdev, if_nametoindex(tun->devname));
+	if (rc < 0)
+		goto err_netdev_free;
+	rc = osmo_netdev_register(tun->netdev);
+	if (rc < 0)
+		goto err_netdev_free;
+	LOGP(DTUN, LOGL_NOTICE, "GTP kernel configured\n");
+	return tun;
+
+err_netdev_free:
+	osmo_netdev_free(tun->netdev);
+err_kernel_create:
+	gtp_kernel_stop(tun->devname);
+err_free:
+	talloc_free(tun);
+	return NULL;
 }
 
 int tun_free(struct tun_t *tun)
 {
-	if (tun->tundev) {
-		if (osmo_tundev_close(tun->tundev) < 0) {
-			SYS_ERR(DTUN, LOGL_ERROR, errno, "close() failed");
+	if (tun->tundev.tundev) {
+		if (osmo_tundev_close(tun->tundev.tundev) < 0) {
+			SYS_ERR(DTUN, LOGL_ERROR, errno, "osmo_tundev_close() failed");
 		}
-		osmo_tundev_free(tun->tundev);
-		tun->tundev = NULL;
+		osmo_tundev_free(tun->tundev.tundev);
+		tun->tundev.tundev = NULL;
 		/* netdev is owned by tundev: */
 		tun->netdev = NULL;
 	} else {
+		gtp_kernel_stop(tun->devname);
 		/* netdev was allocated directly, free it: */
 		osmo_netdev_free(tun->netdev);
 		tun->netdev = NULL;
 	}
 
-	gtp_kernel_stop(tun->devname);
-
 	talloc_free(tun);
 	return 0;
 }
@@ -205,7 +221,7 @@
 	struct msgb *msg;
 	int rc;
 
-	if (!tun->tundev) {
+	if (!tun->tundev.tundev) {
 		LOGTUN(LOGL_ERROR, tun,
 		       "Injecting decapsulated packet not supported in kernel gtp mode: %s\n",
 		       osmo_hexdump(pack, len));
@@ -215,7 +231,7 @@
 	msg = msgb_alloc(PACKET_MAX, "tun_tx");
 	OSMO_ASSERT(msg);
 	memcpy(msgb_put(msg, len), pack, len);
-	rc = osmo_tundev_send(tun->tundev, msg);
+	rc = osmo_tundev_send(tun->tundev.tundev, msg);
 	if (rc < 0) {
 		SYS_ERR(DTUN, LOGL_ERROR, errno, "TUN(%s): write() failed", tun->devname);
 	}
diff --git a/lib/tun.h b/lib/tun.h
index 4d41eb8..eb1bee8 100644
--- a/lib/tun.h
+++ b/lib/tun.h
@@ -33,9 +33,9 @@
  *************************************************************/
 
 struct tun_t {
-	struct osmo_tundev *tundev;
+	/* In tun device mode: operates on the tun interface, owned by tun.tundev below.
+	 * In gtp kernel mode: operates on the gtp device, allocated explicitly */
 	struct osmo_netdev *netdev;
-	int fd;			/* File descriptor to tun interface */
 	struct in46_addr addr;
 	struct in_addr netmask;
 	int addrs;		/* Number of allocated IP addresses */
@@ -43,9 +43,15 @@
 	int (*cb_ind) (struct tun_t * tun, void *pack, unsigned len);
 	/* to be used by libgtp callers/users (to attach their own private state) */
 	void *priv;
+	/* Fields only in use when using the tun device mode: */
+	struct {
+		struct osmo_tundev *tundev; /* Manages the tun interface; NULL on gtp kernel mode */
+		int fd; /* File descriptor to tun interface; -1 on gtp kernel mode */
+	} tundev;
 };
 
-extern int tun_new(struct tun_t **tun, const char *dev_name, bool use_kernel, int fd0, int fd1u);
+extern struct tun_t *tun_alloc_tundev(const char *devname);
+extern struct tun_t *tun_alloc_gtpdev(const char *devname, int fd0, int fd1u);
 extern int tun_free(struct tun_t *tun);
 extern int tun_inject_pkt(struct tun_t *tun, void *pack, unsigned len);
 
diff --git a/sgsnemu/sgsnemu.c b/sgsnemu/sgsnemu.c
index d195fab..afa7339 100644
--- a/sgsnemu/sgsnemu.c
+++ b/sgsnemu/sgsnemu.c
@@ -1905,7 +1905,8 @@
 	if (options.createif) {
 		printf("Setting up interface\n");
 		/* Create a tunnel interface */
-		if (tun_new((struct tun_t **)&tun, options.tun_dev_name, false, -1, -1)) {
+		tun = tun_alloc_tundev(options.tun_dev_name);
+		if (!tun) {
 			SYS_ERR(DSGSN, LOGL_ERROR, 0,
 				"Failed to create tun");
 			exit(1);
@@ -1925,8 +1926,8 @@
 #endif
 
 		tun_set_cb_ind(tun, cb_tun_ind);
-		if (tun->fd > maxfd)
-			maxfd = tun->fd;
+		if (tun->tundev.fd > maxfd)
+			maxfd = tun->tundev.fd;
 
 		if (proc_ipv6_conf_write(tun->devname, "accept_ra", "0") < 0) {
 			SYS_ERR(DSGSN, LOGL_ERROR, 0,
@@ -2159,7 +2160,7 @@
 
 		FD_ZERO(&fds);
 		if (tun)
-			FD_SET(tun->fd, &fds);
+			FD_SET(tun->tundev.fd, &fds);
 		FD_SET(gsn->fd0, &fds);
 		FD_SET(gsn->fd1c, &fds);
 		FD_SET(gsn->fd1u, &fds);
@@ -2187,7 +2188,7 @@
 
 		if (!signal_received) {
 
-			if ((tun) && FD_ISSET(tun->fd, &fds)) {
+			if ((tun) && FD_ISSET(tun->tundev.fd, &fds)) {
 				osmo_select_main(1);
 			}
 

To view, visit change 38524. To unsubscribe, or for help writing mail filters, visit settings.