Richard W.M. Jones
2010-Aug-23 21:00 UTC
[Libguestfs] [PATCH] Use virtio-serial, remove other vmchannel methods.
Notes: (1) Tis doesn't completely remove legacy serial ports yet. I couldn't work out how to make the Linux kernel see the virtconsole, so that is still using a serial port. (2) The guest network is completely unavailable if you apply this patch. We will need to add this back, possibly as an optional feature. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://et.redhat.com/~rjones/virt-top -------------- next part -------------->From c20f8187ac05d334e13064de805b94dd769000ce Mon Sep 17 00:00:00 2001From: Richard Jones <rjones at redhat.com> Date: Mon, 23 Aug 2010 21:53:32 +0100 Subject: [PATCH] Use virtio-serial, remove other vmchannel methods. This adds support for virtio-serial, and removes all other vmchannel methods. Virtio-serial is faster than other methods, and is now widely available. I tested this by using the guestfs_upload API on an 83 MB file: before: 6.12 seconds (14.1 MB/sec) after: 4.20 seconds (20.6 MB/sec) (note this is with the current 8K chunk size) --- README | 3 +- daemon/guestfsd.c | 139 +-------------------- src/guestfs-internal.h | 6 - src/launch.c | 323 +++++++++--------------------------------------- 4 files changed, 63 insertions(+), 408 deletions(-) diff --git a/README b/README index b31f9a1..f128eb5 100644 --- a/README +++ b/README @@ -37,8 +37,7 @@ Home page Requirements ---------------------------------------------------------------------- -- recent QEMU >= 0.10 with vmchannel support - http://lists.gnu.org/archive/html/qemu-devel/2009-02/msg01042.html +- recent QEMU >= 0.12 with virtio-serial support - febootstrap >= 2.8 diff --git a/daemon/guestfsd.c b/daemon/guestfsd.c index 4e29338..8130524 100644 --- a/daemon/guestfsd.c +++ b/daemon/guestfsd.c @@ -56,20 +56,6 @@ static char *read_cmdline (void); -/* This is the default address we connect to for very old libraries - * which didn't specify the address and port number explicitly on the - * kernel command line. It's now recommended to always specify the - * address and port number on the command line, so this will not be - * used any more. - */ -#define OLD_GUESTFWD_ADDR "10.0.2.4" -#define OLD_GUESTFWD_PORT "6666" - -/* This is only a hint. If not defined, ignore it. */ -#ifndef AI_ADDRCONFIG -# define AI_ADDRCONFIG 0 -#endif - #ifndef MAX # define MAX(a,b) ((a)>(b)?(a):(b)) #endif @@ -134,15 +120,14 @@ static void usage (void) { fprintf (stderr, - "guestfsd [-f|--foreground] [-c|--channel vmchannel] [-v|--verbose]\n"); + "guestfsd [-f|--foreground] [-v|--verbose]\n"); } int main (int argc, char *argv[]) { - static const char *options = "fc:v?"; + static const char *options = "fv?"; static const struct option long_options[] = { - { "channel", required_argument, 0, 'c' }, { "foreground", 0, 0, 'f' }, { "help", 0, 0, '?' }, { "verbose", 0, 0, 'v' }, @@ -151,7 +136,6 @@ main (int argc, char *argv[]) int c; int dont_fork = 0; char *cmdline; - char *vmchannel = NULL; if (winsock_init () == -1) error (EXIT_FAILURE, 0, "winsock initialization failed"); @@ -178,10 +162,6 @@ main (int argc, char *argv[]) if (c == -1) break; switch (c) { - case 'c': - vmchannel = optarg; - break; - case 'f': dont_fork = 1; break; @@ -256,118 +236,12 @@ main (int argc, char *argv[]) _umask (0); #endif - /* Get the vmchannel string. - * - * Sources: - * --channel/-c option on the command line - * guestfs_vmchannel=... from the kernel command line - * guestfs=... from the kernel command line - * built-in default - * - * At the moment we expect this to contain "tcp:ip:port" but in - * future it might contain a device name, eg. "/dev/vcon4" for - * virtio-console vmchannel. - */ - if (vmchannel == NULL && cmdline) { - char *p; - size_t len; - - p = strstr (cmdline, "guestfs_vmchannel="); - if (p) { - len = strcspn (p + 18, " \t\n"); - vmchannel = strndup (p + 18, len); - if (!vmchannel) { - perror ("strndup"); - exit (EXIT_FAILURE); - } - } - - /* Old libraries passed guestfs=host:port. Rewrite it as tcp:host:port. */ - if (vmchannel == NULL) { - /* We will rewrite it part of the "guestfs=" string with - * "tcp:" hence p + 4 below. */ - p = strstr (cmdline, "guestfs="); - if (p) { - len = strcspn (p + 4, " \t\n"); - vmchannel = strndup (p + 4, len); - if (!vmchannel) { - perror ("strndup"); - exit (EXIT_FAILURE); - } - memcpy (vmchannel, "tcp:", 4); - } - } - } - - /* Default vmchannel. */ - if (vmchannel == NULL) { - vmchannel = strdup ("tcp:" OLD_GUESTFWD_ADDR ":" OLD_GUESTFWD_PORT); - if (!vmchannel) { - perror ("strdup"); - exit (EXIT_FAILURE); - } - } - - if (verbose) - printf ("vmchannel: %s\n", vmchannel); - - /* Connect to vmchannel. */ - int sock = -1; - - if (STREQLEN (vmchannel, "tcp:", 4)) { - /* Resolve the hostname. */ - struct addrinfo *res, *rr; - struct addrinfo hints; - int r; - char *host, *port; - - host = vmchannel+4; - port = strchr (host, ':'); - if (port) { - port[0] = '\0'; - port++; - } else { - fprintf (stderr, "vmchannel: expecting \"tcp:<ip>:<port>\": %s\n", - vmchannel); - exit (EXIT_FAILURE); - } - - memset (&hints, 0, sizeof hints); - hints.ai_socktype = SOCK_STREAM; - hints.ai_flags = AI_ADDRCONFIG; - r = getaddrinfo (host, port, &hints, &res); - if (r != 0) { - fprintf (stderr, "%s:%s: %s\n", - host, port, gai_strerror (r)); - exit (EXIT_FAILURE); - } - - /* Connect to the given TCP socket. */ - for (rr = res; rr != NULL; rr = rr->ai_next) { - sock = socket (rr->ai_family, rr->ai_socktype, rr->ai_protocol); - if (sock != -1) { - if (connect (sock, rr->ai_addr, rr->ai_addrlen) == 0) - break; - perror ("connect"); - - close (sock); - sock = -1; - } - } - freeaddrinfo (res); - } else { - fprintf (stderr, - "unknown vmchannel connection type: %s\n" - "expecting \"tcp:<ip>:<port>\"\n", - vmchannel); - exit (EXIT_FAILURE); - } - + /* Connect to virtio-serial channel. */ + int sock = open ("/dev/virtio-ports/org.libguestfs.channel.0", O_RDWR); if (sock == -1) { fprintf (stderr, "\n" - "Failed to connect to any vmchannel implementation.\n" - "vmchannel: %s\n" + "Failed to connect to virtio-serial channel.\n" "\n" "This is a fatal error and the appliance will now exit.\n" "\n" @@ -377,8 +251,7 @@ main (int argc, char *argv[]) "'libguestfs-test-tool' and provide the complete, unedited\n" "output to the libguestfs developers, either in a bug report\n" "or on the libguestfs redhat com mailing list.\n" - "\n", - vmchannel); + "\n"); exit (EXIT_FAILURE); } diff --git a/src/guestfs-internal.h b/src/guestfs-internal.h index f2abeba..be915da 100644 --- a/src/guestfs-internal.h +++ b/src/guestfs-internal.h @@ -38,12 +38,6 @@ #define N_(str) str #endif -#ifdef __linux__ -#define CAN_CHECK_PEER_EUID 1 -#else -#define CAN_CHECK_PEER_EUID 0 -#endif - #define UNIX_PATH_MAX 108 #ifndef MAX diff --git a/src/launch.c b/src/launch.c index f84a128..1e1ea8e 100644 --- a/src/launch.c +++ b/src/launch.c @@ -70,7 +70,6 @@ #include "guestfs-internal-actions.h" #include "guestfs_protocol.h" -static int check_peer_euid (guestfs_h *g, int sock, uid_t *rtn); static int qemu_supports (guestfs_h *g, const char *option); /* Add a string to the current command line. */ @@ -233,7 +232,6 @@ guestfs__launch (guestfs_h *g) int r; int wfd[2], rfd[2]; int tries; - int null_vmchannel_sock; char unixsock[256]; struct sockaddr_un addr; @@ -283,53 +281,35 @@ guestfs__launch (guestfs_h *g) if (qemu_supports (g, NULL) == -1) goto cleanup0; - /* Choose which vmchannel implementation to use. */ - if (CAN_CHECK_PEER_EUID && qemu_supports (g, "-net user")) { - /* The "null vmchannel" implementation. Requires SLIRP (user mode - * networking in qemu) but no other vmchannel support. The daemon - * will connect back to a random port number on localhost. - */ - struct sockaddr_in addr; - socklen_t addrlen = sizeof addr; + /* Using virtio-serial, we need to create a local Unix domain socket + * for qemu to connect to. + */ + snprintf (unixsock, sizeof unixsock, "%s/sock", g->tmpdir); + unlink (unixsock); - g->sock = socket (AF_INET, SOCK_STREAM, IPPROTO_TCP); - if (g->sock == -1) { - perrorf (g, "socket"); - goto cleanup0; - } - addr.sin_family = AF_INET; - addr.sin_port = htons (0); - addr.sin_addr.s_addr = htonl (INADDR_LOOPBACK); - if (bind (g->sock, (struct sockaddr *) &addr, addrlen) == -1) { - perrorf (g, "bind"); - goto cleanup0; - } + g->sock = socket (AF_UNIX, SOCK_STREAM, 0); + if (g->sock == -1) { + perrorf (g, "socket"); + goto cleanup0; + } - if (listen (g->sock, 256) == -1) { - perrorf (g, "listen"); - goto cleanup0; - } + if (fcntl (g->sock, F_SETFL, O_NONBLOCK) == -1) { + perrorf (g, "fcntl"); + goto cleanup0; + } - if (getsockname (g->sock, (struct sockaddr *) &addr, &addrlen) == -1) { - perrorf (g, "getsockname"); - goto cleanup0; - } + addr.sun_family = AF_UNIX; + strncpy (addr.sun_path, unixsock, UNIX_PATH_MAX); + addr.sun_path[UNIX_PATH_MAX-1] = '\0'; - if (fcntl (g->sock, F_SETFL, O_NONBLOCK) == -1) { - perrorf (g, "fcntl"); - goto cleanup0; - } + if (bind (g->sock, &addr, sizeof addr) == -1) { + perrorf (g, "bind"); + goto cleanup0; + } - null_vmchannel_sock = ntohs (addr.sin_port); - if (g->verbose) - fprintf (stderr, "null_vmchannel_sock = %d\n", null_vmchannel_sock); - } else { - /* Using some vmchannel impl. We need to create a local Unix - * domain socket for qemu to use. - */ - snprintf (unixsock, sizeof unixsock, "%s/sock", g->tmpdir); - unlink (unixsock); - null_vmchannel_sock = 0; + if (listen (g->sock, 1) == -1) { + perrorf (g, "listen"); + goto cleanup0; } if (!g->direct) { @@ -356,7 +336,6 @@ guestfs__launch (guestfs_h *g) if (r == 0) { /* Child (qemu). */ char buf[256]; - const char *vmchannel = NULL; /* Set up the full command line. Do this in the subprocess so we * don't need to worry about cleaning up. @@ -391,8 +370,6 @@ guestfs__launch (guestfs_h *g) add_cmdline (g, "-nodefaults"); add_cmdline (g, "-nographic"); - add_cmdline (g, "-serial"); - add_cmdline (g, "stdio"); snprintf (buf, sizeof buf, "%d", g->memsize); add_cmdline (g, "-m"); @@ -408,65 +385,30 @@ guestfs__launch (guestfs_h *g) if (qemu_supports (g, "-rtc-td-hack")) add_cmdline (g, "-rtc-td-hack"); - /* If qemu has SLIRP (user mode network) enabled then we can get - * away with "no vmchannel", where we just connect back to a random - * host port. - */ - if (null_vmchannel_sock) { - add_cmdline (g, "-net"); - add_cmdline (g, "user,vlan=0,net=" NETWORK); - - snprintf (buf, sizeof buf, - "guestfs_vmchannel=tcp:" ROUTER ":%d", - null_vmchannel_sock); - vmchannel = strdup (buf); - } - - /* New-style -net user,guestfwd=... syntax for guestfwd. See: - * - * http://git.savannah.gnu.org/cgit/qemu.git/commit/?id=c92ef6a22d3c71538fcc48fb61ad353f7ba03b62 - * - * The original suggested format doesn't work, see: - * - * http://lists.gnu.org/archive/html/qemu-devel/2009-07/msg01654.html - * - * However Gerd Hoffman privately suggested to me using -chardev - * instead, which does work. - */ - else if (qemu_supports (g, "-chardev") && qemu_supports (g, "guestfwd")) { - snprintf (buf, sizeof buf, - "socket,id=guestfsvmc,path=%s,server,nowait", unixsock); - - add_cmdline (g, "-chardev"); - add_cmdline (g, buf); + /* Create the virtio serial bus. */ + add_cmdline (g, "-device"); + add_cmdline (g, "virtio-serial"); - snprintf (buf, sizeof buf, - "user,vlan=0,net=" NETWORK "," - "guestfwd=tcp:" GUESTFWD_ADDR ":" GUESTFWD_PORT - "-chardev:guestfsvmc"); - - add_cmdline (g, "-net"); - add_cmdline (g, buf); - - vmchannel = "guestfs_vmchannel=tcp:" GUESTFWD_ADDR ":" GUESTFWD_PORT; - } - - /* Not guestfwd. HOPEFULLY this qemu uses the older -net channel - * syntax, or if not then we'll get a quick failure. +#if 0 + /* Use virtio-console (a variant form of virtio-serial) for the + * guest's serial console. */ - else { - snprintf (buf, sizeof buf, - "channel," GUESTFWD_PORT ":unix:%s,server,nowait", unixsock); - - add_cmdline (g, "-net"); - add_cmdline (g, buf); - add_cmdline (g, "-net"); - add_cmdline (g, "user,vlan=0,net=" NETWORK); + add_cmdline (g, "-chardev"); + add_cmdline (g, "stdio,id=console"); + add_cmdline (g, "-device"); + add_cmdline (g, "virtconsole,chardev=console,name=org.libguestfs.console.0"); +#else + /* When the above works ... until then: */ + add_cmdline (g, "-serial"); + add_cmdline (g, "stdio"); +#endif - vmchannel = "guestfs_vmchannel=tcp:" GUESTFWD_ADDR ":" GUESTFWD_PORT; - } - add_cmdline (g, "-net"); - add_cmdline (g, "nic,model=" NET_IF ",vlan=0"); + /* Set up virtio-serial for the communications channel. */ + add_cmdline (g, "-chardev"); + snprintf (buf, sizeof buf, "socket,path=%s,id=channel0", unixsock); + add_cmdline (g, buf); + add_cmdline (g, "-device"); + add_cmdline (g, "virtserialport,chardev=channel0,name=org.libguestfs.channel.0"); #define LINUX_CMDLINE \ "panic=1 " /* force kernel to panic if daemon exits */ \ @@ -481,12 +423,10 @@ guestfs__launch (guestfs_h *g) snprintf (buf, sizeof buf, LINUX_CMDLINE "%s " /* (selinux) */ - "%s " /* (vmchannel) */ "%s " /* (verbose) */ "TERM=%s " /* (TERM environment variable) */ "%s", /* (append) */ g->selinux ? "selinux=1 enforcing=0" : "selinux=0", - vmchannel ? vmchannel : "", g->verbose ? "guestfs_verbose=1" : "", getenv ("TERM") ? : "linux", g->append ? g->append : ""); @@ -627,90 +567,23 @@ guestfs__launch (guestfs_h *g) } } - if (null_vmchannel_sock) { - int sock = -1; - uid_t uid; - - /* Null vmchannel implementation: We listen on g->sock for a - * connection. The connection could come from any local process - * so we must check it comes from the appliance (or at least - * from our UID) for security reasons. - */ - while (sock == -1) { - sock = guestfs___accept_from_daemon (g); - if (sock == -1) - goto cleanup1; - - if (check_peer_euid (g, sock, &uid) == -1) - goto cleanup1; - if (uid != geteuid ()) { - fprintf (stderr, - "libguestfs: warning: unexpected connection from UID %d to port %d\n", - uid, null_vmchannel_sock); - close (sock); - sock = -1; - continue; - } - } - - if (fcntl (sock, F_SETFL, O_NONBLOCK) == -1) { - perrorf (g, "fcntl"); - goto cleanup1; - } - - close (g->sock); - g->sock = sock; - } else { - /* Other vmchannel. Open the Unix socket. - * - * The vmchannel implementation that got merged with qemu sucks in - * a number of ways. Both ends do connect(2), which means that no - * one knows what, if anything, is connected to the other end, or - * if it becomes disconnected. Even worse, we have to wait some - * indeterminate time for qemu to create the socket and connect to - * it (which happens very early in qemu's start-up), so any code - * that uses vmchannel is inherently racy. Hence this silly loop. - */ - g->sock = socket (AF_UNIX, SOCK_STREAM, 0); - if (g->sock == -1) { - perrorf (g, "socket"); - goto cleanup1; - } + g->state = LAUNCHING; - if (fcntl (g->sock, F_SETFL, O_NONBLOCK) == -1) { - perrorf (g, "fcntl"); - goto cleanup1; - } + /* Wait for qemu to start and to connect back to us via + * virtio-serial and send the GUESTFS_LAUNCH_FLAG message. + */ + r = guestfs___accept_from_daemon (g); + if (r == -1) + goto cleanup1; - addr.sun_family = AF_UNIX; - strncpy (addr.sun_path, unixsock, UNIX_PATH_MAX); - addr.sun_path[UNIX_PATH_MAX-1] = '\0'; - - tries = 100; - /* Always sleep at least once to give qemu a small chance to start up. */ - usleep (10000); - while (tries > 0) { - r = connect (g->sock, (struct sockaddr *) &addr, sizeof addr); - if ((r == -1 && errno == EINPROGRESS) || r == 0) - goto connected; - - if (errno != ENOENT) - perrorf (g, "connect"); - tries--; - usleep (100000); - } + close (g->sock); /* Close the listening socket. */ + g->sock = r; /* This is the accepted data socket. */ - error (g, _("failed to connect to vmchannel socket")); + if (fcntl (g->sock, F_SETFL, O_NONBLOCK) == -1) { + perrorf (g, "fcntl"); goto cleanup1; - - connected: ; } - g->state = LAUNCHING; - - /* Wait for qemu to start and to connect back to us via vmchannel and - * send the GUESTFS_LAUNCH_FLAG message. - */ uint32_t size; void *buf = NULL; r = guestfs___recv_from_daemon (g, &size, &buf); @@ -954,90 +827,6 @@ is_openable (guestfs_h *g, const char *path, int flags) return 1; } -/* Check the peer effective UID for a TCP socket. Ideally we'd like - * SO_PEERCRED for a loopback TCP socket. This isn't possible on - * Linux (but it is on Solaris!) so we read /proc/net/tcp instead. - */ -static int -check_peer_euid (guestfs_h *g, int sock, uid_t *rtn) -{ -#if CAN_CHECK_PEER_EUID - struct sockaddr_in peer; - socklen_t addrlen = sizeof peer; - - if (getpeername (sock, (struct sockaddr *) &peer, &addrlen) == -1) { - perrorf (g, "getpeername"); - return -1; - } - - if (peer.sin_family != AF_INET || - ntohl (peer.sin_addr.s_addr) != INADDR_LOOPBACK) { - error (g, "check_peer_euid: unexpected connection from non-IPv4, non-loopback peer (family = %d, addr = %s)", - peer.sin_family, inet_ntoa (peer.sin_addr)); - return -1; - } - - struct sockaddr_in our; - addrlen = sizeof our; - if (getsockname (sock, (struct sockaddr *) &our, &addrlen) == -1) { - perrorf (g, "getsockname"); - return -1; - } - - FILE *fp = fopen ("/proc/net/tcp", "r"); - if (fp == NULL) { - perrorf (g, "/proc/net/tcp"); - return -1; - } - - char line[256]; - if (fgets (line, sizeof line, fp) == NULL) { /* Drop first line. */ - error (g, "unexpected end of file in /proc/net/tcp"); - fclose (fp); - return -1; - } - - while (fgets (line, sizeof line, fp) != NULL) { - unsigned line_our_addr, line_our_port, line_peer_addr, line_peer_port; - int dummy0, dummy1, dummy2, dummy3, dummy4, dummy5, dummy6; - int line_uid; - - if (sscanf (line, "%d:%08X:%04X %08X:%04X %02X %08X:%08X %02X:%08X %08X %d", - &dummy0, - &line_our_addr, &line_our_port, - &line_peer_addr, &line_peer_port, - &dummy1, &dummy2, &dummy3, &dummy4, &dummy5, &dummy6, - &line_uid) == 12) { - /* Note about /proc/net/tcp: local_address and rem_address are - * always in network byte order. However the port part is - * always in host byte order. - * - * The sockname and peername that we got above are in network - * byte order. So we have to byte swap the port but not the - * address part. - */ - if (line_our_addr == our.sin_addr.s_addr && - line_our_port == ntohs (our.sin_port) && - line_peer_addr == peer.sin_addr.s_addr && - line_peer_port == ntohs (peer.sin_port)) { - *rtn = line_uid; - fclose (fp); - return 0; - } - } - } - - error (g, "check_peer_euid: no matching TCP connection found in /proc/net/tcp"); - fclose (fp); - return -1; -#else /* !CAN_CHECK_PEER_EUID */ - /* This function exists but should never be called in this - * configuration. - */ - abort (); -#endif /* !CAN_CHECK_PEER_EUID */ -} - /* You had to call this function after launch in versions <= 1.0.70, * but it is now a no-op. */ -- 1.7.1
Richard W.M. Jones
2010-Aug-23 21:09 UTC
[Libguestfs] [PATCH] Use virtio-serial, remove other vmchannel methods.
On Mon, Aug 23, 2010 at 10:00:52PM +0100, Richard W.M. Jones wrote:> (note this is with the current 8K chunk size)As Matt did, I also tried adjusting the chunk size, but larger sizes just seemed to make it slower. I don't understand why this is. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming blog: http://rwmj.wordpress.com Fedora now supports 80 OCaml packages (the OPEN alternative to F#) http://cocan.org/getting_started_with_ocaml_on_red_hat_and_fedora
Matthew Booth
2010-Aug-24 09:03 UTC
[Libguestfs] [PATCH] Use virtio-serial, remove other vmchannel methods.
On 23/08/10 22:00, Richard W.M. Jones wrote:> (1) Tis doesn't completely remove legacy serial ports yet. I couldn't > work out how to make the Linux kernel see the virtconsole, so that is > still using a serial port. > > (2) The guest network is completely unavailable if you apply this > patch. We will need to add this back, possibly as an optional > feature.This looks good, but 'need' is the operative word in your second point. This patch can't be merged, except as part of a series which adds guest networking back in. Matt -- Matthew Booth, RHCA, RHCSS Red Hat Engineering, Virtualisation Team GPG ID: D33C3490 GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490