diff mbox

[v3,1/4] Add basic version of bridge helper

Message ID 1320086191-23641-2-git-send-email-coreyb@linux.vnet.ibm.com
State New
Headers show

Commit Message

Corey Bryant Oct. 31, 2011, 6:36 p.m. UTC 
This patch adds a helper that can be used to create a tap device attached to
a bridge device.  Since this helper is minimal in what it does, it can be
given CAP_NET_ADMIN which allows qemu to avoid running as root while still
satisfying the majority of what users tend to want to do with tap devices.

The way this all works is that qemu launches this helper passing a bridge
name and the name of an inherited file descriptor.  The descriptor is one
end of a socketpair() of domain sockets.  This domain socket is used to
transmit a file descriptor of the opened tap device from the helper to qemu.

The helper can then exit and let qemu use the tap device.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
Signed-off-by: Richa Marwaha <rmarwah@linux.vnet.ibm.com>
Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com>
---
 Makefile             |   12 +++-
 configure            |    1 +
 qemu-bridge-helper.c |  217 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 228 insertions(+), 2 deletions(-)
 create mode 100644 qemu-bridge-helper.c

Comments

Stefan Hajnoczi Nov. 1, 2011, 8:15 a.m. UTC | #1 
On Mon, Oct 31, 2011 at 02:36:28PM -0400, Corey Bryant wrote:
A couple of nitpicks regarding error handling:

> +static int has_vnet_hdr(int fd)
> +{
> +    unsigned int features = 0;
> +    struct ifreq ifreq;
> +
> +    if (ioctl(fd, TUNGETFEATURES, &features) == -1) {
> +        return -errno;
> +    }
> +
> +    if (!(features & IFF_VNET_HDR)) {
> +        return -ENOTSUP;
> +    }
> +
> +    if (ioctl(fd, TUNGETIFF, &ifreq) != -1 || errno != EBADFD) {
> +        return -ENOTSUP;
> +    }
> +
> +    return 1;
> +}

This function is strange, it looks like a boolean function but actually
only returns 1 or -errno.  It is used incorrectly in main().  I suggest
changing the return value to bool and returning false on error.

> +    /* open a socket to use to control the network interfaces */
> +    ctlfd = socket(AF_INET, SOCK_STREAM, 0);
> +    if (ctlfd == -1) {
> +        fprintf(stderr, "failed to open control socket\n");
> +        ret = -errno;

It's better to stash away errno before invoking other library functions.
man errno(3) says:

"a function that succeeds is allowed to change errno"

This means fprintf(3) could clobber errno.

I suggest simply printing out errno with the error message and returning
exit code 1 (EXIT_FAILURE).  The same applies for the other error exit
cases in main().

> +cleanup:
> +
> +    close(fd);
> +
> +    close(ctlfd);

ctlfd is an uninitialized variable if opening fd fails.  We also never
close unixfd.

I'd remove this cleanup code and just return without closing any file
descriptors - let the kernel do it.

Stefan
Corey Bryant Nov. 1, 2011, 1:52 p.m. UTC | #2 
On 11/01/2011 04:15 AM, Stefan Hajnoczi wrote:
> On Mon, Oct 31, 2011 at 02:36:28PM -0400, Corey Bryant wrote:
> A couple of nitpicks regarding error handling:
>
>> +static int has_vnet_hdr(int fd)
>> +{
>> +    unsigned int features = 0;
>> +    struct ifreq ifreq;
>> +
>> +    if (ioctl(fd, TUNGETFEATURES,&features) == -1) {
>> +        return -errno;
>> +    }
>> +
>> +    if (!(features&  IFF_VNET_HDR)) {
>> +        return -ENOTSUP;
>> +    }
>> +
>> +    if (ioctl(fd, TUNGETIFF,&ifreq) != -1 || errno != EBADFD) {
>> +        return -ENOTSUP;
>> +    }
>> +
>> +    return 1;
>> +}
>
> This function is strange, it looks like a boolean function but actually
> only returns 1 or -errno.  It is used incorrectly in main().  I suggest
> changing the return value to bool and returning false on error.
>

Ah, good catch, this was a bug.  And I agree that bool would work 
better.  I'll fix this.

>> +    /* open a socket to use to control the network interfaces */
>> +    ctlfd = socket(AF_INET, SOCK_STREAM, 0);
>> +    if (ctlfd == -1) {
>> +        fprintf(stderr, "failed to open control socket\n");
>> +        ret = -errno;
>
> It's better to stash away errno before invoking other library functions.
> man errno(3) says:
>
> "a function that succeeds is allowed to change errno"
>
> This means fprintf(3) could clobber errno.
>
> I suggest simply printing out errno with the error message and returning
> exit code 1 (EXIT_FAILURE).  The same applies for the other error exit
> cases in main().
>

I agree.  I'll fix this.

>> +cleanup:
>> +
>> +    close(fd);
>> +
>> +    close(ctlfd);
>
> ctlfd is an uninitialized variable if opening fd fails.  We also never
> close unixfd.
>
> I'd remove this cleanup code and just return without closing any file
> descriptors - let the kernel do it.

Ok, I'll do this.  But I think I'll re-introduce the cleanup goto in 
patch 2/4 to free the simple queue memory.
diff mbox

Patch

diff --git a/Makefile b/Makefile
index ba8d738..46b6838 100644
--- a/Makefile
+++ b/Makefile
@@ -35,6 +35,8 @@  $(call set-vpath, $(SRC_PATH):$(SRC_PATH)/hw)
 
 LIBS+=-lz $(LIBS_TOOLS)
 
+HELPERS-$(CONFIG_LINUX) = qemu-bridge-helper$(EXESUF)
+
 ifdef BUILD_DOCS
 DOCS=qemu-doc.html qemu-tech.html qemu.1 qemu-img.1 qemu-nbd.8 QMP/qmp-commands.txt
 else
@@ -75,7 +77,7 @@  defconfig:
 
 -include config-all-devices.mak
 
-build-all: $(DOCS) $(TOOLS) recurse-all
+build-all: $(DOCS) $(TOOLS) $(HELPERS-y) recurse-all
 
 config-host.h: config-host.h-timestamp
 config-host.h-timestamp: config-host.mak
@@ -153,6 +155,8 @@  qemu-img$(EXESUF): qemu-img.o $(tools-obj-y) $(block-obj-y)
 qemu-nbd$(EXESUF): qemu-nbd.o $(tools-obj-y) $(block-obj-y)
 qemu-io$(EXESUF): qemu-io.o cmd.o $(tools-obj-y) $(block-obj-y)
 
+qemu-bridge-helper$(EXESUF): qemu-bridge-helper.o
+
 qemu-img-cmds.h: $(SRC_PATH)/qemu-img-cmds.hx
 	$(call quiet-command,sh $(SRC_PATH)/scripts/hxtool -h < $< > $@,"  GEN   $@")
 
@@ -219,7 +223,7 @@  clean:
 # avoid old build problems by removing potentially incorrect old files
 	rm -f config.mak op-i386.h opc-i386.h gen-op-i386.h op-arm.h opc-arm.h gen-op-arm.h
 	rm -f qemu-options.def
-	rm -f *.o *.d *.a *.lo $(TOOLS) qemu-ga TAGS cscope.* *.pod *~ */*~
+	rm -f *.o *.d *.a *.lo $(TOOLS) $(HELPERS-y) qemu-ga TAGS cscope.* *.pod *~ */*~
 	rm -Rf .libs
 	rm -f slirp/*.o slirp/*.d audio/*.o audio/*.d block/*.o block/*.d net/*.o net/*.d fsdev/*.o fsdev/*.d ui/*.o ui/*.d qapi/*.o qapi/*.d qga/*.o qga/*.d
 	rm -f qemu-img-cmds.h
@@ -287,6 +291,10 @@  install: all $(if $(BUILD_DOCS),install-doc) install-sysconfig
 ifneq ($(TOOLS),)
 	$(INSTALL_PROG) $(STRIP_OPT) $(TOOLS) "$(DESTDIR)$(bindir)"
 endif
+ifneq ($(HELPERS-y),)
+	$(INSTALL_DIR) "$(DESTDIR)$(libexecdir)"
+	$(INSTALL_PROG) $(STRIP_OPT) $(HELPERS-y) "$(DESTDIR)$(libexecdir)"
+endif
 ifneq ($(BLOBS),)
 	$(INSTALL_DIR) "$(DESTDIR)$(datadir)"
 	set -e; for x in $(BLOBS); do \
diff --git a/configure b/configure
index a6cf6d6..94c89a7 100755
--- a/configure
+++ b/configure
@@ -2795,6 +2795,7 @@  echo "datadir=$datadir" >> $config_host_mak
 echo "sysconfdir=$sysconfdir" >> $config_host_mak
 echo "docdir=$docdir" >> $config_host_mak
 echo "confdir=$confdir" >> $config_host_mak
+echo "libexecdir=\${prefix}/libexec" >> $config_host_mak
 
 case "$cpu" in
   i386|x86_64|alpha|cris|hppa|ia64|lm32|m68k|microblaze|mips|mips64|ppc|ppc64|s390|s390x|sparc|sparc64|unicore32)
diff --git a/qemu-bridge-helper.c b/qemu-bridge-helper.c
new file mode 100644
index 0000000..803681d
--- /dev/null
+++ b/qemu-bridge-helper.c
@@ -0,0 +1,217 @@ 
+/*
+ * QEMU Bridge Helper
+ *
+ * Copyright IBM, Corp. 2011
+ *
+ * Authors:
+ * Anthony Liguori   <aliguori@us.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ *
+ */
+
+#include "config-host.h"
+
+#include <stdio.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <unistd.h>
+#include <string.h>
+#include <stdlib.h>
+#include <ctype.h>
+
+#include <sys/types.h>
+#include <sys/ioctl.h>
+#include <sys/socket.h>
+#include <sys/un.h>
+#include <sys/prctl.h>
+
+#include <net/if.h>
+
+#include <linux/sockios.h>
+
+#include "net/tap-linux.h"
+
+static int has_vnet_hdr(int fd)
+{
+    unsigned int features = 0;
+    struct ifreq ifreq;
+
+    if (ioctl(fd, TUNGETFEATURES, &features) == -1) {
+        return -errno;
+    }
+
+    if (!(features & IFF_VNET_HDR)) {
+        return -ENOTSUP;
+    }
+
+    if (ioctl(fd, TUNGETIFF, &ifreq) != -1 || errno != EBADFD) {
+        return -ENOTSUP;
+    }
+
+    return 1;
+}
+
+static void prep_ifreq(struct ifreq *ifr, const char *ifname)
+{
+    memset(ifr, 0, sizeof(*ifr));
+    snprintf(ifr->ifr_name, IFNAMSIZ, "%s", ifname);
+}
+
+static int send_fd(int c, int fd)
+{
+    char msgbuf[CMSG_SPACE(sizeof(fd))];
+    struct msghdr msg = {
+        .msg_control = msgbuf,
+        .msg_controllen = sizeof(msgbuf),
+    };
+    struct cmsghdr *cmsg;
+    struct iovec iov;
+    char req[1] = { 0x00 };
+
+    cmsg = CMSG_FIRSTHDR(&msg);
+    cmsg->cmsg_level = SOL_SOCKET;
+    cmsg->cmsg_type = SCM_RIGHTS;
+    cmsg->cmsg_len = CMSG_LEN(sizeof(fd));
+    msg.msg_controllen = cmsg->cmsg_len;
+
+    iov.iov_base = req;
+    iov.iov_len = sizeof(req);
+
+    msg.msg_iov = &iov;
+    msg.msg_iovlen = 1;
+    memcpy(CMSG_DATA(cmsg), &fd, sizeof(fd));
+
+    return sendmsg(c, &msg, 0);
+}
+
+int main(int argc, char **argv)
+{
+    struct ifreq ifr;
+    int fd, ctlfd, unixfd;
+    int use_vnet = 0;
+    int mtu;
+    const char *bridge;
+    char iface[IFNAMSIZ];
+    int index;
+    int ret = 0;
+
+    /* parse arguments */
+    if (argc < 3 || argc > 4) {
+        fprintf(stderr, "Usage: %s [--use-vnet] BRIDGE FD\n", argv[0]);
+        return 1;
+    }
+
+    index = 1;
+    if (strcmp(argv[index], "--use-vnet") == 0) {
+        use_vnet = 1;
+        index++;
+        if (argc == 3) {
+            fprintf(stderr, "invalid number of arguments\n");
+            return -1;
+        }
+    }
+
+    bridge = argv[index++];
+    unixfd = atoi(argv[index++]);
+
+    /* open a socket to use to control the network interfaces */
+    ctlfd = socket(AF_INET, SOCK_STREAM, 0);
+    if (ctlfd == -1) {
+        fprintf(stderr, "failed to open control socket\n");
+        ret = -errno;
+        goto cleanup;
+    }
+
+    /* open the tap device */
+    fd = open("/dev/net/tun", O_RDWR);
+    if (fd == -1) {
+        fprintf(stderr, "failed to open /dev/net/tun\n");
+        ret = -errno;
+        goto cleanup;
+    }
+
+    /* request a tap device, disable PI, and add vnet header support if
+     * requested and it's available. */
+    prep_ifreq(&ifr, "tap%d");
+    ifr.ifr_flags = IFF_TAP|IFF_NO_PI;
+    if (use_vnet && has_vnet_hdr(fd)) {
+        ifr.ifr_flags |= IFF_VNET_HDR;
+    }
+
+    if (ioctl(fd, TUNSETIFF, &ifr) == -1) {
+        fprintf(stderr, "failed to create tun device\n");
+        ret = -errno;
+        goto cleanup;
+    }
+
+    /* save tap device name */
+    snprintf(iface, sizeof(iface), "%s", ifr.ifr_name);
+
+    /* get the mtu of the bridge */
+    prep_ifreq(&ifr, bridge);
+    if (ioctl(ctlfd, SIOCGIFMTU, &ifr) == -1) {
+        fprintf(stderr, "failed to get mtu of bridge `%s'\n", bridge);
+        ret = -errno;
+        goto cleanup;
+    }
+
+    /* save mtu */
+    mtu = ifr.ifr_mtu;
+
+    /* set the mtu of the interface based on the bridge */
+    prep_ifreq(&ifr, iface);
+    ifr.ifr_mtu = mtu;
+    if (ioctl(ctlfd, SIOCSIFMTU, &ifr) == -1) {
+        fprintf(stderr, "failed to set mtu of device `%s' to %d\n",
+                iface, mtu);
+        ret = -errno;
+        goto cleanup;
+    }
+
+    /* add the interface to the bridge */
+    prep_ifreq(&ifr, bridge);
+    ifr.ifr_ifindex = if_nametoindex(iface);
+
+    if (ioctl(ctlfd, SIOCBRADDIF, &ifr) == -1) {
+        fprintf(stderr, "failed to add interface `%s' to bridge `%s'\n",
+                iface, bridge);
+        ret = -errno;
+        goto cleanup;
+    }
+
+    /* bring the interface up */
+    prep_ifreq(&ifr, iface);
+    if (ioctl(ctlfd, SIOCGIFFLAGS, &ifr) == -1) {
+        fprintf(stderr, "failed to get interface flags for `%s'\n", iface);
+        ret = -errno;
+        goto cleanup;
+    }
+
+    ifr.ifr_flags |= IFF_UP;
+    if (ioctl(ctlfd, SIOCSIFFLAGS, &ifr) == -1) {
+        fprintf(stderr, "failed to set bring up interface `%s'\n", iface);
+        ret = -errno;
+        goto cleanup;
+    }
+
+    /* write fd to the domain socket */
+    if (send_fd(unixfd, fd) == -1) {
+        fprintf(stderr, "failed to write fd to unix socket\n");
+        ret = -errno;
+        goto cleanup;
+    }
+
+    /* ... */
+
+    /* profit! */
+
+cleanup:
+
+    close(fd);
+
+    close(ctlfd);
+
+    return ret;
+}