xgetty: Smart serial console getty for Xen

xgetty: Smart serial console getty for Xen

[Daniel Kiper]

Hi,

Very often when I was switching from Xen to Linux I was
forced to change /etc/inittab to make serial console
working. It was boring so I thought how to solve that
problem. I was not able to find sensible solution. So
I decided to write something. Here it is.

If you find it useful could we add this to Xen 4.4?
I know that now it could be too late but I think that
it is nice feature and it does not touch core stuff so
it should not break anything important.

Daniel

[PATCH 1/2] xgetty: Do not edit /etc/inittab when switching from Xen to Linux and back

[Daniel Kiper]

xgetty establishes serial console name from system boot command
line (/proc/cmdline) and then runs getty. This is very useful if
someone switches very often between Linux and Xen and uses serial
console. xgetty does boring things and runs getty on relevant line.
This way nobody needs to worry about /etc/inittab changes anymore.

Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
---
 tools/Makefile        |    1 +
 tools/xgetty/Makefile |   17 ++++
 tools/xgetty/xgetty.c |  254 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 272 insertions(+)
 create mode 100644 tools/xgetty/Makefile
 create mode 100644 tools/xgetty/xgetty.c

diff --git a/tools/Makefile b/tools/Makefile
index 00c69ee..55b3c69 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -25,6 +25,7 @@ SUBDIRS-$(CONFIG_NetBSD) += xenbackendd
 SUBDIRS-y += libfsimage
 SUBDIRS-$(LIBXENAPI_BINDINGS) += libxen
 SUBDIRS-$(CONFIG_Linux) += libvchan
+SUBDIRS-$(CONFIG_Linux) += xgetty
 
 # do not recurse in to a dir we are about to delete
 ifneq "$(MAKECMDGOALS)" "distclean"
diff --git a/tools/xgetty/Makefile b/tools/xgetty/Makefile
new file mode 100644
index 0000000..fde57c0
--- /dev/null
+++ b/tools/xgetty/Makefile
@@ -0,0 +1,17 @@
+XEN_ROOT=$(CURDIR)/../..
+
+include $(XEN_ROOT)/tools/Rules.mk
+
+.PHONY: all clean install
+
+all: xgetty
+
+xgetty: xgetty.o
+	$(CC) $(LDFLAGS) -o $@ $^
+
+install: all
+	[ -d $(DESTDIR)$(SBINDIR) ] || $(INSTALL_DIR) $(DESTDIR)$(SBINDIR)
+	$(INSTALL_PROG) xgetty $(DESTDIR)$(SBINDIR)/xgetty
+
+clean:
+	rm -fr $(DEPS) *.o xgetty
diff --git a/tools/xgetty/xgetty.c b/tools/xgetty/xgetty.c
new file mode 100644
index 0000000..ca3a594
--- /dev/null
+++ b/tools/xgetty/xgetty.c
@@ -0,0 +1,254 @@
+/*
+ * Copyright (c) 2013 Daniel Kiper, Oracle Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <errno.h>
+#include <libgen.h>
+#include <stdarg.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <syslog.h>
+#include <unistd.h>
+
+#define BOOT_CMDLINE	"/proc/cmdline"
+
+#define GETTY		"/sbin/getty"
+
+static char *xgetty_file = "xgetty";
+
+static char *opt_b = NULL;
+static int opt_c = 1;
+static char *opt_g = GETTY;
+static char *opt_t = NULL;
+
+static void usage(int status)
+{
+    FILE *stream;
+
+    stream = ( status == EXIT_SUCCESS ) ? stdout : stderr;
+
+    fprintf(stream, "\nxgetty Ver. 1.0.0\n\n"
+		    "xgetty establishes serial console name from system boot command\n"
+		    "line (%s) and then runs getty. This is very useful if\n"
+		    "someone switches very often between Linux and Xen and uses serial\n"
+		    "console. xgetty does boring things and runs getty on relevant line.\n"
+		    "This way nobody needs to worry about /etc/inittab changes anymore.\n\n"
+		    "Usage: xgetty [<options>] [-- [<getty_options>]]\n\n"
+		    "Options:\n"
+		    "  -b <baud_rate> - baud rate (required),\n"
+		    "  -c <console> - serial console number in system boot\n"
+		    "                 command line (default: 1),\n"
+		    "  -g <getty> - path to getty (default: %s),\n"
+		    "  -h - display this help,\n"
+		    "  -t <term> - terminal type.\n\n"
+		    "<getty_options> are passed without any changes to getty.\n\n"
+		    "Warning: do not pass a line name, baud rate and terminal type\n"
+		    "in <getty_options>. Line name is established by xgetty.\n"
+		    "Baud rate and terminal type (if needed) must be\n"
+		    "passed via relevant xgetty options.\n\n"
+		    "xgetty takes into account ttyS*, hvc* and xvc* only\n"
+		    "in system boot command line.\n\n"
+		    "Example: xgetty -g /sbin/agetty -b 115200 -c 2 -- -i\n\n",
+		    BOOT_CMDLINE, GETTY);
+
+      exit(status);
+}
+
+static void do_log(int priority, const char *format, ...)
+{
+    va_list ap;
+
+    va_start(ap, format);
+
+    openlog(xgetty_file, LOG_PID, LOG_AUTHPRIV);
+    vsyslog(priority, format, ap);
+    closelog();
+
+    va_end(ap);
+}
+
+static void init(int argc, char *const argv[])
+{
+    char *endptr, *s;
+    int c;
+
+    s = basename(argv[0]);
+
+    if ( strcmp(s, ".") && strcmp(s, "/") )
+	xgetty_file = s;
+
+    opterr = 0;
+
+    while ( (c = getopt(argc, argv, "+b:c:g:ht:")) != -1 )
+	switch (c)
+	{
+	case 'b':
+	    opt_b = optarg;
+	    break;
+
+	case 'c':
+	    errno = 0;
+	    opt_c = strtol(optarg, &endptr, 10);
+
+	    if ( errno || *endptr || opt_c < 1 )
+	    {
+		fprintf(stderr, "Wrong serial console number\n");
+		do_log(LOG_WARNING, "Wrong serial console number");
+		usage(EXIT_FAILURE);
+	    }
+
+	    break;
+
+	case 'g':
+	    opt_g = optarg;
+	    break;
+
+	case 'h':
+	    usage(EXIT_SUCCESS);
+
+	case 't':
+	    opt_t = optarg;
+	    break;
+
+	default:
+	    fprintf(stderr, "Wrong arguments\n");
+	    do_log(LOG_WARNING, "Wrong arguments");
+	    usage(EXIT_FAILURE);
+	}
+
+    if ( !opt_b )
+    {
+	fprintf(stderr, "Baud rate is not specified\n");
+	do_log(LOG_ERR, "Baud rate is not specified");
+	usage(EXIT_FAILURE);
+    }
+}
+
+static char *get_console_name(void)
+{
+    FILE *stream;
+    char *boot_cmdline = NULL, *tok;
+    int i = 1;
+    size_t n;
+    ssize_t rc;
+
+    stream = fopen(BOOT_CMDLINE, "r");
+
+    if ( !stream )
+    {
+	do_log(LOG_ERR, "Cannot open file: %s: %m", BOOT_CMDLINE);
+	exit(EXIT_FAILURE);
+    }
+
+    rc = getline(&boot_cmdline, &n, stream);
+
+    if ( rc == -1 )
+    {
+	do_log(LOG_ERR, "Cannot read file: %s: %m", BOOT_CMDLINE);
+	exit(EXIT_FAILURE);
+    }
+
+    fclose(stream);
+
+    tok = strtok(boot_cmdline, " \n\r\t");
+
+    while ( tok )
+    {
+	if ( strstr(tok, "console=ttyS") == tok ||
+	     strstr(tok, "console=hvc") == tok ||
+	     strstr(tok, "console=xvc") == tok)
+	{
+	    if ( i == opt_c )
+	    {
+		tok += strlen("console=");
+		return strtok(tok, ", \n\r\t");
+	    }
+
+	    if ( ++i > opt_c )
+		return NULL;
+	}
+
+	tok = strtok(NULL, " \n\r\t");
+    }
+
+    return NULL;
+}
+
+static void exec_getty(int argc, char *const argv[], char *console)
+{
+    char **g_argv;
+    /* Path to getty, console name, baud rate and final NULL. */
+    int g_argc = 4;
+    int i = 0, j = optind;
+
+    if ( !console )
+    {
+	do_log(LOG_INFO, "Serial console not found. Nothing to do. "
+			 "I am going sleep... Yawn...");
+	sleep(86400); /* Sleep one day... */
+	exit(EXIT_SUCCESS);
+    }
+
+    /* How many plain getty options do we have? */
+    g_argc += argc - optind;
+
+    /* Add space for terminal type if needed. */
+    if ( opt_t )
+	++g_argc;
+
+    g_argv = calloc(g_argc, sizeof(char *));
+
+    if ( !g_argv )
+    {
+	do_log(LOG_ERR, "Cannot allocate memory for getty args: %m");
+	exit(EXIT_FAILURE);
+    }
+
+    /* Path to getty. */
+    g_argv[i++] = opt_g;
+
+    /* Plain options to getty. */
+    while ( j < argc )
+	g_argv[i++] = argv[j++];
+
+    /* Console name. */
+    g_argv[i++] = console;
+
+    /* Baud rate. */
+    g_argv[i++] = opt_b;
+
+    /* Terminal type if available. */
+    if ( opt_t )
+	g_argv[i] = opt_t;
+
+    /* Run getty. */
+    execv(opt_g, g_argv);
+
+    /* Ugh... Something went wrong... Dying... */
+    do_log(LOG_ERR, "Cannot execute %s: %m", opt_g);
+}
+
+int main(int argc, char *const argv[])
+{
+    char *console;
+
+    init(argc, argv);
+    console = get_console_name();
+    exec_getty(argc, argv, console);
+
+    return EXIT_FAILURE;
+}
-- 
1.7.10.4

[PATCH 2/2] MAINTAINERS: Add XGETTY maintainer

[Daniel Kiper]

Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
---
 MAINTAINERS |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 33130e5..1808bf3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -345,6 +345,12 @@ F:  xen/include/xsm/
 F:  xen/xsm/
 F:  docs/misc/xsm-flask.txt
 
+XGETTY
+M:	Daniel Kiper <daniel.kiper@oracle.com>
+S:	Supported
+L:	xen-devel@lists.xen.org
+F:	tools/xgetty
+
 THE REST
 M:	Keir Fraser <keir@xen.org>
 L:	xen-devel@lists.xen.org
-- 
1.7.10.4

Re: [PATCH 1/2] xgetty: Do not edit /etc/inittab when switching from Xen to Linux and back

[Ian Campbell]

On Thu, 2013-11-14 at 12:44 +0100, Daniel Kiper wrote:
> xgetty establishes serial console name from system boot command
> line (/proc/cmdline) and then runs getty. This is very useful if
> someone switches very often between Linux and Xen and uses serial
> console. xgetty does boring things and runs getty on relevant line.
> This way nobody needs to worry about /etc/inittab changes anymore.

This seems like it could be very useful, but it doesn't seem at all Xen
specific. I think it would be worth trying to get this into an upstream
-- e.g. agetty (since that is what you appear to shell out to).

We've been slowly getting rid of the various random tools (e.g. losetup,
miniterm), I'd be loathe to start adding more.

Ian.

> 
> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
> ---
>  tools/Makefile        |    1 +
>  tools/xgetty/Makefile |   17 ++++
>  tools/xgetty/xgetty.c |  254 +++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 272 insertions(+)
>  create mode 100644 tools/xgetty/Makefile
>  create mode 100644 tools/xgetty/xgetty.c
> 
> diff --git a/tools/Makefile b/tools/Makefile
> index 00c69ee..55b3c69 100644
> --- a/tools/Makefile
> +++ b/tools/Makefile
> @@ -25,6 +25,7 @@ SUBDIRS-$(CONFIG_NetBSD) += xenbackendd
>  SUBDIRS-y += libfsimage
>  SUBDIRS-$(LIBXENAPI_BINDINGS) += libxen
>  SUBDIRS-$(CONFIG_Linux) += libvchan
> +SUBDIRS-$(CONFIG_Linux) += xgetty
>  
>  # do not recurse in to a dir we are about to delete
>  ifneq "$(MAKECMDGOALS)" "distclean"
> diff --git a/tools/xgetty/Makefile b/tools/xgetty/Makefile
> new file mode 100644
> index 0000000..fde57c0
> --- /dev/null
> +++ b/tools/xgetty/Makefile
> @@ -0,0 +1,17 @@
> +XEN_ROOT=$(CURDIR)/../..
> +
> +include $(XEN_ROOT)/tools/Rules.mk
> +
> +.PHONY: all clean install
> +
> +all: xgetty
> +
> +xgetty: xgetty.o
> +	$(CC) $(LDFLAGS) -o $@ $^
> +
> +install: all
> +	[ -d $(DESTDIR)$(SBINDIR) ] || $(INSTALL_DIR) $(DESTDIR)$(SBINDIR)
> +	$(INSTALL_PROG) xgetty $(DESTDIR)$(SBINDIR)/xgetty
> +
> +clean:
> +	rm -fr $(DEPS) *.o xgetty
> diff --git a/tools/xgetty/xgetty.c b/tools/xgetty/xgetty.c
> new file mode 100644
> index 0000000..ca3a594
> --- /dev/null
> +++ b/tools/xgetty/xgetty.c
> @@ -0,0 +1,254 @@
> +/*
> + * Copyright (c) 2013 Daniel Kiper, Oracle Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <errno.h>
> +#include <libgen.h>
> +#include <stdarg.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <syslog.h>
> +#include <unistd.h>
> +
> +#define BOOT_CMDLINE	"/proc/cmdline"
> +
> +#define GETTY		"/sbin/getty"
> +
> +static char *xgetty_file = "xgetty";
> +
> +static char *opt_b = NULL;
> +static int opt_c = 1;
> +static char *opt_g = GETTY;
> +static char *opt_t = NULL;
> +
> +static void usage(int status)
> +{
> +    FILE *stream;
> +
> +    stream = ( status == EXIT_SUCCESS ) ? stdout : stderr;
> +
> +    fprintf(stream, "\nxgetty Ver. 1.0.0\n\n"
> +		    "xgetty establishes serial console name from system boot command\n"
> +		    "line (%s) and then runs getty. This is very useful if\n"
> +		    "someone switches very often between Linux and Xen and uses serial\n"
> +		    "console. xgetty does boring things and runs getty on relevant line.\n"
> +		    "This way nobody needs to worry about /etc/inittab changes anymore.\n\n"
> +		    "Usage: xgetty [<options>] [-- [<getty_options>]]\n\n"
> +		    "Options:\n"
> +		    "  -b <baud_rate> - baud rate (required),\n"
> +		    "  -c <console> - serial console number in system boot\n"
> +		    "                 command line (default: 1),\n"
> +		    "  -g <getty> - path to getty (default: %s),\n"
> +		    "  -h - display this help,\n"
> +		    "  -t <term> - terminal type.\n\n"
> +		    "<getty_options> are passed without any changes to getty.\n\n"
> +		    "Warning: do not pass a line name, baud rate and terminal type\n"
> +		    "in <getty_options>. Line name is established by xgetty.\n"
> +		    "Baud rate and terminal type (if needed) must be\n"
> +		    "passed via relevant xgetty options.\n\n"
> +		    "xgetty takes into account ttyS*, hvc* and xvc* only\n"
> +		    "in system boot command line.\n\n"
> +		    "Example: xgetty -g /sbin/agetty -b 115200 -c 2 -- -i\n\n",
> +		    BOOT_CMDLINE, GETTY);
> +
> +      exit(status);
> +}
> +
> +static void do_log(int priority, const char *format, ...)
> +{
> +    va_list ap;
> +
> +    va_start(ap, format);
> +
> +    openlog(xgetty_file, LOG_PID, LOG_AUTHPRIV);
> +    vsyslog(priority, format, ap);
> +    closelog();
> +
> +    va_end(ap);
> +}
> +
> +static void init(int argc, char *const argv[])
> +{
> +    char *endptr, *s;
> +    int c;
> +
> +    s = basename(argv[0]);
> +
> +    if ( strcmp(s, ".") && strcmp(s, "/") )
> +	xgetty_file = s;
> +
> +    opterr = 0;
> +
> +    while ( (c = getopt(argc, argv, "+b:c:g:ht:")) != -1 )
> +	switch (c)
> +	{
> +	case 'b':
> +	    opt_b = optarg;
> +	    break;
> +
> +	case 'c':
> +	    errno = 0;
> +	    opt_c = strtol(optarg, &endptr, 10);
> +
> +	    if ( errno || *endptr || opt_c < 1 )
> +	    {
> +		fprintf(stderr, "Wrong serial console number\n");
> +		do_log(LOG_WARNING, "Wrong serial console number");
> +		usage(EXIT_FAILURE);
> +	    }
> +
> +	    break;
> +
> +	case 'g':
> +	    opt_g = optarg;
> +	    break;
> +
> +	case 'h':
> +	    usage(EXIT_SUCCESS);
> +
> +	case 't':
> +	    opt_t = optarg;
> +	    break;
> +
> +	default:
> +	    fprintf(stderr, "Wrong arguments\n");
> +	    do_log(LOG_WARNING, "Wrong arguments");
> +	    usage(EXIT_FAILURE);
> +	}
> +
> +    if ( !opt_b )
> +    {
> +	fprintf(stderr, "Baud rate is not specified\n");
> +	do_log(LOG_ERR, "Baud rate is not specified");
> +	usage(EXIT_FAILURE);
> +    }
> +}
> +
> +static char *get_console_name(void)
> +{
> +    FILE *stream;
> +    char *boot_cmdline = NULL, *tok;
> +    int i = 1;
> +    size_t n;
> +    ssize_t rc;
> +
> +    stream = fopen(BOOT_CMDLINE, "r");
> +
> +    if ( !stream )
> +    {
> +	do_log(LOG_ERR, "Cannot open file: %s: %m", BOOT_CMDLINE);
> +	exit(EXIT_FAILURE);
> +    }
> +
> +    rc = getline(&boot_cmdline, &n, stream);
> +
> +    if ( rc == -1 )
> +    {
> +	do_log(LOG_ERR, "Cannot read file: %s: %m", BOOT_CMDLINE);
> +	exit(EXIT_FAILURE);
> +    }
> +
> +    fclose(stream);
> +
> +    tok = strtok(boot_cmdline, " \n\r\t");
> +
> +    while ( tok )
> +    {
> +	if ( strstr(tok, "console=ttyS") == tok ||
> +	     strstr(tok, "console=hvc") == tok ||
> +	     strstr(tok, "console=xvc") == tok)
> +	{
> +	    if ( i == opt_c )
> +	    {
> +		tok += strlen("console=");
> +		return strtok(tok, ", \n\r\t");
> +	    }
> +
> +	    if ( ++i > opt_c )
> +		return NULL;
> +	}
> +
> +	tok = strtok(NULL, " \n\r\t");
> +    }
> +
> +    return NULL;
> +}
> +
> +static void exec_getty(int argc, char *const argv[], char *console)
> +{
> +    char **g_argv;
> +    /* Path to getty, console name, baud rate and final NULL. */
> +    int g_argc = 4;
> +    int i = 0, j = optind;
> +
> +    if ( !console )
> +    {
> +	do_log(LOG_INFO, "Serial console not found. Nothing to do. "
> +			 "I am going sleep... Yawn...");
> +	sleep(86400); /* Sleep one day... */
> +	exit(EXIT_SUCCESS);
> +    }
> +
> +    /* How many plain getty options do we have? */
> +    g_argc += argc - optind;
> +
> +    /* Add space for terminal type if needed. */
> +    if ( opt_t )
> +	++g_argc;
> +
> +    g_argv = calloc(g_argc, sizeof(char *));
> +
> +    if ( !g_argv )
> +    {
> +	do_log(LOG_ERR, "Cannot allocate memory for getty args: %m");
> +	exit(EXIT_FAILURE);
> +    }
> +
> +    /* Path to getty. */
> +    g_argv[i++] = opt_g;
> +
> +    /* Plain options to getty. */
> +    while ( j < argc )
> +	g_argv[i++] = argv[j++];
> +
> +    /* Console name. */
> +    g_argv[i++] = console;
> +
> +    /* Baud rate. */
> +    g_argv[i++] = opt_b;
> +
> +    /* Terminal type if available. */
> +    if ( opt_t )
> +	g_argv[i] = opt_t;
> +
> +    /* Run getty. */
> +    execv(opt_g, g_argv);
> +
> +    /* Ugh... Something went wrong... Dying... */
> +    do_log(LOG_ERR, "Cannot execute %s: %m", opt_g);
> +}
> +
> +int main(int argc, char *const argv[])
> +{
> +    char *console;
> +
> +    init(argc, argv);
> +    console = get_console_name();
> +    exec_getty(argc, argv, console);
> +
> +    return EXIT_FAILURE;
> +}

Re: [PATCH 1/2] xgetty: Do not edit /etc/inittab when switching from Xen to Linux and back

[Daniel Kiper]

On Thu, Nov 14, 2013 at 11:51:05AM +0000, Ian Campbell wrote:
> On Thu, 2013-11-14 at 12:44 +0100, Daniel Kiper wrote:
> > xgetty establishes serial console name from system boot command
> > line (/proc/cmdline) and then runs getty. This is very useful if
> > someone switches very often between Linux and Xen and uses serial
> > console. xgetty does boring things and runs getty on relevant line.
> > This way nobody needs to worry about /etc/inittab changes anymore.
>
> This seems like it could be very useful, but it doesn't seem at all Xen
> specific. I think it would be worth trying to get this into an upstream
> -- e.g. agetty (since that is what you appear to shell out to).
>
> We've been slowly getting rid of the various random tools (e.g. losetup,
> miniterm), I'd be loathe to start adding more.

Thanks. Roger.

Daniel

Re: [PATCH 1/2] xgetty: Do not edit /etc/inittab when switching from Xen to Linux and back

[George Dunlap]

On 14/11/13 11:51, Ian Campbell wrote:
> On Thu, 2013-11-14 at 12:44 +0100, Daniel Kiper wrote:
>> xgetty establishes serial console name from system boot command
>> line (/proc/cmdline) and then runs getty. This is very useful if
>> someone switches very often between Linux and Xen and uses serial
>> console. xgetty does boring things and runs getty on relevant line.
>> This way nobody needs to worry about /etc/inittab changes anymore.
> This seems like it could be very useful, but it doesn't seem at all Xen
> specific. I think it would be worth trying to get this into an upstream
> -- e.g. agetty (since that is what you appear to shell out to).

I've been using an inittab entry like this:

T0:23:respawn:bash -c "/sbin/getty --noclear -L 
$(/etc/find-remote-tty.sh) 115200 vt102"

But having agetty able to search the Linux command-line for the argument 
is much better.

  -George