pespin has submitted this change. ( https://gerrit.osmocom.org/c/osmo-ggsn/+/38524?usp=email )
Change subject: Refactor tun_t fields and alloc APIs ......................................................................
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(-)
Approvals: Jenkins Builder: Verified pespin: Looks good to me, approved osmith: Looks good to me, but someone else must approve fixeria: Looks good to me, but someone else must approve
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); }
-
pespin