--- cfg.mk | 2 +- po/POTFILES.in | 2 +- tools/Makefile.am | 2 +- tools/{console.c => virsh-console.c} | 73 ++++++++++++++++++++++-------------- tools/{console.h => virsh-console.h} | 4 +- tools/virsh-domain.c | 2 +- tools/virsh.c | 2 +- 7 files changed, 52 insertions(+), 35 deletions(-) rename tools/{console.c => virsh-console.c} (90%) rename tools/{console.h => virsh-console.h} (91%) diff --git a/cfg.mk b/cfg.mk index 23564f1..5c3f3ab 100644 --- a/cfg.mk +++ b/cfg.mk @@ -907,7 +907,7 @@ exclude_file_name_regexp--sc_avoid_strcase = ^tools/virsh\.h$$ _src1=libvirt|fdstream|qemu/qemu_monitor|util/(vircommand|virfile)|xen/xend_internal|rpc/virnetsocket|lxc/lxc_controller|locking/lock_daemon _test1=shunloadtest|virnettlscontexttest|virnettlssessiontest|vircgroupmock exclude_file_name_regexp--sc_avoid_write = \ - ^(src/($(_src1))|daemon/libvirtd|tools/console|tests/($(_test1)))\.c$$ + ^(src/($(_src1))|daemon/libvirtd|tools/virsh-console|tests/($(_test1)))\.c$$ exclude_file_name_regexp--sc_bindtextdomain = ^(tests|examples)/ diff --git a/po/POTFILES.in b/po/POTFILES.in index 9a83069..aa604fd 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -213,10 +213,10 @@ src/xenapi/xenapi_driver.c src/xenapi/xenapi_utils.c src/xenxs/xen_sxpr.c src/xenxs/xen_xm.c -tools/console.c tools/libvirt-guests.sh.in tools/virsh.c tools/virsh.h +tools/virsh-console.c tools/virsh-domain-monitor.c tools/virsh-domain.c tools/virsh-edit.c diff --git a/tools/Makefile.am b/tools/Makefile.am index 74fae2d..cd4cb31 100644 --- a/tools/Makefile.am +++ b/tools/Makefile.am @@ -161,8 +161,8 @@ virt_login_shell_CFLAGS = \ $(COVERAGE_CFLAGS) virsh_SOURCES = \ - console.c console.h \ virsh.c virsh.h \ + virsh-console.c virsh-console.h \ virsh-domain.c virsh-domain.h \ virsh-domain-monitor.c virsh-domain-monitor.h \ virsh-host.c virsh-host.h \ diff --git a/tools/console.c b/tools/virsh-console.c similarity index 90% rename from tools/console.c rename to tools/virsh-console.c index 6c24fcf..debf12c 100644 --- a/tools/console.c +++ b/tools/virsh-console.c @@ -1,7 +1,7 @@ /* - * console.c: A dumb serial console client + * virsh-console.c: A dumb serial console client * - * Copyright (C) 2007-2008, 2010-2012 Red Hat, Inc. + * Copyright (C) 2007-2008, 2010-2013 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -37,7 +37,7 @@ # include <c-ctype.h> # include "internal.h" -# include "console.h" +# include "virsh-console.h" # include "virlog.h" # include "virfile.h" # include "viralloc.h" @@ -58,6 +58,7 @@ struct virConsoleBuffer { char *data; }; + typedef struct virConsole virConsole; typedef virConsole *virConsolePtr; struct virConsole { @@ -75,11 +76,15 @@ struct virConsole { char escapeChar; }; + static int got_signal = 0; -static void do_signal(int sig ATTRIBUTE_UNUSED) { +static void +virConsoleHandleSignal(int sig ATTRIBUTE_UNUSED) +{ got_signal = 1; } + # ifndef HAVE_CFMAKERAW static void cfmakeraw(struct termios *attr) @@ -93,6 +98,7 @@ cfmakeraw(struct termios *attr) } # endif /* !HAVE_CFMAKERAW */ + static void virConsoleShutdown(virConsolePtr con) { @@ -114,6 +120,21 @@ virConsoleShutdown(virConsolePtr con) virCondSignal(&con->cond); } + +static void +virConsoleFree(virConsolePtr con) +{ + if (!con) + return; + + if (con->st) + virStreamFree(con->st); + virMutexDestroy(&con->lock); + virCondDestroy(&con->cond); + VIR_FREE(con); +} + + static void virConsoleEventOnStream(virStreamPtr st, int events, void *opaque) @@ -171,9 +192,8 @@ virConsoleEventOnStream(virStreamPtr st, avail = con->terminalToStream.length - con->terminalToStream.offset; if (avail > 1024) { - if (VIR_REALLOC_N(con->terminalToStream.data, - con->terminalToStream.offset + 1024) < 0) - {} + ignore_value(VIR_REALLOC_N(con->terminalToStream.data, + con->terminalToStream.offset + 1024)); con->terminalToStream.length = con->terminalToStream.offset + 1024; } } @@ -187,6 +207,7 @@ virConsoleEventOnStream(virStreamPtr st, } } + static void virConsoleEventOnStdin(int watch ATTRIBUTE_UNUSED, int fd ATTRIBUTE_UNUSED, @@ -242,6 +263,7 @@ virConsoleEventOnStdin(int watch ATTRIBUTE_UNUSED, } } + static void virConsoleEventOnStdout(int watch ATTRIBUTE_UNUSED, int fd, @@ -270,9 +292,8 @@ virConsoleEventOnStdout(int watch ATTRIBUTE_UNUSED, avail = con->streamToTerminal.length - con->streamToTerminal.offset; if (avail > 1024) { - if (VIR_REALLOC_N(con->streamToTerminal.data, - con->streamToTerminal.offset + 1024) < 0) - {} + ignore_value(VIR_REALLOC_N(con->streamToTerminal.data, + con->streamToTerminal.offset + 1024)); con->streamToTerminal.length = con->streamToTerminal.offset + 1024; } } @@ -296,6 +317,7 @@ vshGetEscapeChar(const char *s) return *s; } + int vshMakeStdinRaw(struct termios *ttyattr, bool report_errors) { @@ -322,10 +344,12 @@ vshMakeStdinRaw(struct termios *ttyattr, bool report_errors) return 0; } -int vshRunConsole(virDomainPtr dom, - const char *dev_name, - const char *escape_seq, - unsigned int flags) + +int +vshRunConsole(virDomainPtr dom, + const char *dev_name, + const char *escape_seq, + unsigned int flags) { int ret = -1; struct termios ttyattr; @@ -348,11 +372,11 @@ int vshRunConsole(virDomainPtr dom, the original terminal settings on STDIN before the process exits - people don't like being left with a messed up terminal ! */ - old_sigquit = signal(SIGQUIT, do_signal); - old_sigterm = signal(SIGTERM, do_signal); - old_sigint = signal(SIGINT, do_signal); - old_sighup = signal(SIGHUP, do_signal); - old_sigpipe = signal(SIGPIPE, do_signal); + old_sigquit = signal(SIGQUIT, virConsoleHandleSignal); + old_sigterm = signal(SIGTERM, virConsoleHandleSignal); + old_sigint = signal(SIGINT, virConsoleHandleSignal); + old_sighup = signal(SIGHUP, virConsoleHandleSignal); + old_sigpipe = signal(SIGPIPE, virConsoleHandleSignal); got_signal = 0; if (VIR_ALLOC(con) < 0) @@ -396,15 +420,8 @@ int vshRunConsole(virDomainPtr dom, ret = 0; - cleanup: - - if (con) { - if (con->st) - virStreamFree(con->st); - virMutexDestroy(&con->lock); - virCondDestroy(&con->cond); - VIR_FREE(con); - } +cleanup: + virConsoleFree(con); /* Restore original signal handlers */ signal(SIGPIPE, old_sigpipe); diff --git a/tools/console.h b/tools/virsh-console.h similarity index 91% rename from tools/console.h rename to tools/virsh-console.h index 46255b8..96ef235 100644 --- a/tools/console.h +++ b/tools/virsh-console.h @@ -1,7 +1,7 @@ /* - * console.c: A dumb serial console client + * virsh-console.h: A dumb serial console client * - * Copyright (C) 2007, 2010, 2012 Red Hat, Inc. + * Copyright (C) 2007, 2010, 2012-2013 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index c87cf6a..60eed51 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -41,7 +41,7 @@ #include "virbuffer.h" #include "c-ctype.h" #include "conf/domain_conf.h" -#include "console.h" +#include "virsh-console.h" #include "viralloc.h" #include "vircommand.h" #include "virfile.h" diff --git a/tools/virsh.c b/tools/virsh.c index 2f04e6a..0cc9bdd 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -57,7 +57,6 @@ #include "virerror.h" #include "base64.h" #include "virbuffer.h" -#include "console.h" #include "viralloc.h" #include "virxml.h" #include <libvirt/libvirt-qemu.h> @@ -73,6 +72,7 @@ #include "virtypedparam.h" #include "virstring.h" +#include "virsh-console.h" #include "virsh-domain.h" #include "virsh-domain-monitor.h" #include "virsh-host.h" -- 1.8.3.2

On 09/03/13 11:44, Michal Privoznik wrote: ... Okay, added. ... This is actually decreasing the size of the allocated memory. It's hard to see here, but if there's more than 1024 bytes left free in the buffer, the size is decreased to exactly 1024 extra bytes. Thus this call should be always safe and I just changed the way the result from VIR_REALLOC is ignored. ... Okay, done. Fixed. And pushed. Peter

Move the function to virsh.c to the rest of the TTY managing functions and change the code so that it mirrors the rest. --- tools/virsh-console.c | 50 +++++--------------------------------------------- tools/virsh-console.h | 7 +++---- tools/virsh-domain.c | 2 +- tools/virsh.c | 48 +++++++++++++++++++++++++++++++++++++++++++++--- tools/virsh.h | 1 + 5 files changed, 55 insertions(+), 53 deletions(-) diff --git a/tools/virsh-console.c b/tools/virsh-console.c index debf12c..cc9cc6a 100644 --- a/tools/virsh-console.c +++ b/tools/virsh-console.c @@ -37,6 +37,7 @@ # include <c-ctype.h> # include "internal.h" +# include "virsh.h" # include "virsh-console.h" # include "virlog.h" # include "virfile.h" @@ -85,20 +86,6 @@ virConsoleHandleSignal(int sig ATTRIBUTE_UNUSED) } -# ifndef HAVE_CFMAKERAW -static void -cfmakeraw(struct termios *attr) -{ - attr->c_iflag &= ~(IGNBRK | BRKINT | PARMRK | ISTRIP - | INLCR | IGNCR | ICRNL | IXON); - attr->c_oflag &= ~OPOST; - attr->c_lflag &= ~(ECHO | ECHONL | ICANON | ISIG | IEXTEN); - attr->c_cflag &= ~(CSIZE | PARENB); - attr->c_cflag |= CS8; -} -# endif /* !HAVE_CFMAKERAW */ - - static void virConsoleShutdown(virConsolePtr con) { @@ -319,40 +306,13 @@ vshGetEscapeChar(const char *s) int -vshMakeStdinRaw(struct termios *ttyattr, bool report_errors) -{ - struct termios rawattr; - char ebuf[1024]; - - if (tcgetattr(STDIN_FILENO, ttyattr) < 0) { - if (report_errors) - VIR_ERROR(_("unable to get tty attributes: %s"), - virStrerror(errno, ebuf, sizeof(ebuf))); - return -1; - } - - rawattr = *ttyattr; - cfmakeraw(&rawattr); - - if (tcsetattr(STDIN_FILENO, TCSAFLUSH, &rawattr) < 0) { - if (report_errors) - VIR_ERROR(_("unable to set tty attributes: %s"), - virStrerror(errno, ebuf, sizeof(ebuf))); - return -1; - } - - return 0; -} - - -int -vshRunConsole(virDomainPtr dom, +vshRunConsole(vshControl *ctl, + virDomainPtr dom, const char *dev_name, const char *escape_seq, unsigned int flags) { int ret = -1; - struct termios ttyattr; void (*old_sigquit)(int); void (*old_sigterm)(int); void (*old_sigint)(int); @@ -365,7 +325,7 @@ vshRunConsole(virDomainPtr dom, result in it being echoed back already), and also ensure Ctrl-C, etc is blocked, and misc other bits */ - if (vshMakeStdinRaw(&ttyattr, true) < 0) + if (vshTTYMakeRaw(ctl, true) < 0) goto resettty; /* Trap all common signals so that we can safely restore @@ -433,7 +393,7 @@ cleanup: resettty: /* Put STDIN back into the (sane?) state we found it in before starting */ - tcsetattr(STDIN_FILENO, TCSAFLUSH, &ttyattr); + vshTTYRestore(ctl); return ret; } diff --git a/tools/virsh-console.h b/tools/virsh-console.h index 96ef235..d6dbedd 100644 --- a/tools/virsh-console.h +++ b/tools/virsh-console.h @@ -25,15 +25,14 @@ # ifndef WIN32 -# include <termios.h> +# include <virsh.h> -int vshRunConsole(virDomainPtr dom, +int vshRunConsole(vshControl *ctl, + virDomainPtr dom, const char *dev_name, const char *escape_seq, unsigned int flags); -int vshMakeStdinRaw(struct termios *ttyattr, bool report_errors); - # endif /* !WIN32 */ #endif /* __VIR_CONSOLE_H__ */ diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 60eed51..7a60e48 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -2263,7 +2263,7 @@ cmdRunConsole(vshControl *ctl, virDomainPtr dom, vshPrintExtra(ctl, _("Connected to domain %s\n"), virDomainGetName(dom)); vshPrintExtra(ctl, _("Escape character is %s\n"), ctl->escapeChar); fflush(stdout); - if (vshRunConsole(dom, name, ctl->escapeChar, flags) == 0) + if (vshRunConsole(ctl, dom, name, ctl->escapeChar, flags) == 0) ret = true; cleanup: diff --git a/tools/virsh.c b/tools/virsh.c index 0cc9bdd..37e9716 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -458,14 +458,13 @@ int vshAskReedit(vshControl *ctl, const char *msg) { int c = -1; - struct termios ttyattr; if (!isatty(STDIN_FILENO)) return -1; vshReportError(ctl); - if (vshMakeStdinRaw(&ttyattr, false) < 0) + if (vshTTYMakeRaw(ctl, false) < 0) return -1; while (true) { @@ -488,7 +487,7 @@ vshAskReedit(vshControl *ctl, const char *msg) } } - tcsetattr(STDIN_FILENO, TCSAFLUSH, &ttyattr); + vshTTYRestore(ctl); vshPrint(ctl, "\r\n"); return c; @@ -2260,6 +2259,49 @@ vshTTYRestore(vshControl *ctl) } +#ifndef HAVE_CFMAKERAW +/* provide fallback in case cfmakeraw isn't available */ +static void +cfmakeraw(struct termios *attr) +{ + attr->c_iflag &= ~(IGNBRK | BRKINT | PARMRK | ISTRIP + | INLCR | IGNCR | ICRNL | IXON); + attr->c_oflag &= ~OPOST; + attr->c_lflag &= ~(ECHO | ECHONL | ICANON | ISIG | IEXTEN); + attr->c_cflag &= ~(CSIZE | PARENB); + attr->c_cflag |= CS8; +} +#endif /* !HAVE_CFMAKERAW */ + + +int +vshTTYMakeRaw(vshControl *ctl, bool report_errors) +{ + struct termios rawattr = ctl->termattr; + char ebuf[1024]; + + if (!ctl->istty) { + if (report_errors) { + vshError(ctl, "%s", + _("unable to make terminal raw: console isn't a tty")); + } + + return -1; + } + + cfmakeraw(&rawattr); + + if (tcsetattr(STDIN_FILENO, TCSAFLUSH, &rawattr) < 0) { + if (report_errors) + vshError(ctl, _("unable to set tty attributes: %s"), + virStrerror(errno, ebuf, sizeof(ebuf))); + return -1; + } + + return 0; +} + + void vshError(vshControl *ctl, const char *format, ...) { diff --git a/tools/virsh.h b/tools/virsh.h index db5934f..8afe13f 100644 --- a/tools/virsh.h +++ b/tools/virsh.h @@ -358,6 +358,7 @@ void vshSaveLibvirtError(void); bool vshTTYIsInterruptCharacter(vshControl *ctl, const char chr); int vshTTYDisableInterrupt(vshControl *ctl); int vshTTYRestore(vshControl *ctl); +int vshTTYMakeRaw(vshControl *ctl, bool report_errors); /* allocation wrappers */ void *_vshMalloc(vshControl *ctl, size_t sz, const char *filename, int line); -- 1.8.3.2

On 09/03/13 12:02, Martin Kletzander wrote: Right, I removed the parameter and pushed the rest of the series. Thanks. Peter

Man page for signal states: "The effects of signal() in a multithreaded process are unspecified." Switch signal() to sigaction in virsh console code. --- tools/virsh-console.c | 52 ++++++++++++++++++++++++++------------------------- 1 file changed, 27 insertions(+), 25 deletions(-) diff --git a/tools/virsh-console.c b/tools/virsh-console.c index cc9cc6a..135202f 100644 --- a/tools/virsh-console.c +++ b/tools/virsh-console.c @@ -312,32 +312,34 @@ vshRunConsole(vshControl *ctl, const char *escape_seq, unsigned int flags) { - int ret = -1; - void (*old_sigquit)(int); - void (*old_sigterm)(int); - void (*old_sigint)(int); - void (*old_sighup)(int); - void (*old_sigpipe)(int); virConsolePtr con = NULL; + int ret = -1; + + struct sigaction old_sigquit; + struct sigaction old_sigterm; + struct sigaction old_sigint; + struct sigaction old_sighup; + struct sigaction old_sigpipe; + struct sigaction sighandler = {.sa_handler = virConsoleHandleSignal, + .sa_flags = SA_SIGINFO }; + + sigemptyset(&sighandler.sa_mask); - /* Put STDIN into raw mode so that stuff typed - does not echo to the screen (the TTY reads will - result in it being echoed back already), and - also ensure Ctrl-C, etc is blocked, and misc - other bits */ + /* Put STDIN into raw mode so that stuff typed does not echo to the screen + * (the TTY reads will result in it being echoed back already), and also + * ensure Ctrl-C, etc is blocked, and misc other bits */ if (vshTTYMakeRaw(ctl, true) < 0) goto resettty; - /* Trap all common signals so that we can safely restore - the original terminal settings on STDIN before the - process exits - people don't like being left with a - messed up terminal ! */ - old_sigquit = signal(SIGQUIT, virConsoleHandleSignal); - old_sigterm = signal(SIGTERM, virConsoleHandleSignal); - old_sigint = signal(SIGINT, virConsoleHandleSignal); - old_sighup = signal(SIGHUP, virConsoleHandleSignal); - old_sigpipe = signal(SIGPIPE, virConsoleHandleSignal); + /* Trap all common signals so that we can safely restore the original + * terminal settings on STDIN before the process exits - people don't like + * being left with a messed up terminal ! */ got_signal = 0; + sigaction(SIGQUIT, &sighandler, &old_sigquit); + sigaction(SIGTERM, &sighandler, &old_sigterm); + sigaction(SIGINT, &sighandler, &old_sigint); + sigaction(SIGHUP, &sighandler, &old_sighup); + sigaction(SIGPIPE, &sighandler, &old_sigpipe); if (VIR_ALLOC(con) < 0) goto cleanup; @@ -384,11 +386,11 @@ cleanup: virConsoleFree(con); /* Restore original signal handlers */ - signal(SIGPIPE, old_sigpipe); - signal(SIGHUP, old_sighup); - signal(SIGINT, old_sigint); - signal(SIGTERM, old_sigterm); - signal(SIGQUIT, old_sigquit); + sigaction(SIGQUIT, &old_sigquit, NULL); + sigaction(SIGTERM, &old_sigterm, NULL); + sigaction(SIGINT, &old_sigint, NULL); + sigaction(SIGHUP, &old_sighup, NULL); + sigaction(SIGPIPE, &old_sigpipe, NULL); resettty: /* Put STDIN back into the (sane?) state we found -- 1.8.3.2