[libvirt] [PATCH 0/3] Fix and clean up multiple issues in virsh console handling
This series has to be applied on top of "virsh: Handle interrupting of jobs manually". Peter Krempa (3): tools: rename console.[ch] to virsh-console.[ch] and fix coding style virsh: Rename vshMakeStdinRaw to vshTTYMakeRaw and move it to virsh.c virsh-console: Avoid using signal() in multithreaded application cfg.mk | 2 +- po/POTFILES.in | 2 +- tools/Makefile.am | 2 +- tools/{console.c => virsh-console.c} | 153 +++++++++++++++-------------------- tools/{console.h => virsh-console.h} | 11 ++- tools/virsh-domain.c | 4 +- tools/virsh.c | 50 +++++++++++- tools/virsh.h | 1 + 8 files changed, 123 insertions(+), 102 deletions(-) rename tools/{console.c => virsh-console.c} (76%) rename tools/{console.h => virsh-console.h} (81%) -- 1.8.3.2
--- 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 29.08.2013 18:36, Peter Krempa wrote:
--- 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
Not shown in the context, but there should be 'Authors:' line just above Dan's name.
@@ -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;
I don't think this is quite right. If the VIR_REALLOC fails, why are we increasing the .lenght? I know this is pre-existing, but if we are touching this we should fix this.
} } @@ -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;
And again.
} } @@ -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
Not shown in the context, but there should be 'Authors:' line just above Dan's name.
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"
This should go a few lines down, just before the line: #include "virsh-domain-monitor.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"
I've pointed a few issues but I believe that you will fix them right so I don't need to see a v2. ACK Michal
On 09/03/13 11:44, Michal Privoznik wrote:
On 29.08.2013 18:36, Peter Krempa wrote:
--- 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/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
Not shown in the context, but there should be 'Authors:' line just above Dan's name.
Okay, added. ...
@@ -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;
I don't think this is quite right. If the VIR_REALLOC fails, why are we increasing the .lenght? I know this is pre-existing, but if we are touching this we should fix this.
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.
} }
...
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
Not shown in the context, but there should be 'Authors:' line just above Dan's name.
Okay, done.
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"
This should go a few lines down, just before the line:
Fixed.
#include "virsh-domain-monitor.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"
I've pointed a few issues but I believe that you will fix them right so I don't need to see a v2.
ACK
Michal
And pushed. Peter
On 08/29/2013 06:36 PM, Peter Krempa wrote:
--- 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%)
ACK
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 08/29/2013 06:36 PM, Peter Krempa wrote:
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 [...] @@ -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,
You can get rid of this one since we call it with ctl->excapeChar only. The rest looks ok, but Michal already ACK'd it, so... Martin
On 09/03/13 12:02, Martin Kletzander wrote:
On 08/29/2013 06:36 PM, Peter Krempa wrote:
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 [...] @@ -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,
You can get rid of this one since we call it with ctl->excapeChar only.
The rest looks ok, but Michal already ACK'd it, so...
Right, I removed the parameter and pushed the rest of the series. Thanks. Peter
On 08/29/2013 10:36 AM, Peter Krempa wrote:
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(-)
As we guessed on IRC, this and similar patches have broken the build for mingw, which lacks most terminal control functions. I'm working on a patch that uses #ifndef WIN32 to skip the code that doesn't work on mingw (I don't know if mingw can ever reliably use ssh connections, or if it is better off relying on tls certificates for authentication instead). In file included from ../../tools/virsh-domain-monitor.h:29:0, from ../../tools/virsh-domain-monitor.c:27: ../../tools/virsh.h:245:20: error: field 'termattr' has incomplete type struct termios termattr; /* settings of the tty terminal */ ^ In file included from ../../tools/virsh-host.h:29:0, from ../../tools/virsh-host.c:27: ... -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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
On 29.08.2013 18:36, Peter Krempa wrote:
This series has to be applied on top of "virsh: Handle interrupting of jobs manually".
Peter Krempa (3): tools: rename console.[ch] to virsh-console.[ch] and fix coding style virsh: Rename vshMakeStdinRaw to vshTTYMakeRaw and move it to virsh.c virsh-console: Avoid using signal() in multithreaded application
cfg.mk | 2 +- po/POTFILES.in | 2 +- tools/Makefile.am | 2 +- tools/{console.c => virsh-console.c} | 153 +++++++++++++++-------------------- tools/{console.h => virsh-console.h} | 11 ++- tools/virsh-domain.c | 4 +- tools/virsh.c | 50 +++++++++++- tools/virsh.h | 1 + 8 files changed, 123 insertions(+), 102 deletions(-) rename tools/{console.c => virsh-console.c} (76%) rename tools/{console.h => virsh-console.h} (81%)
ACK series, but see my comments to 1/3. Michal
participants (4)
-
Eric Blake -
Martin Kletzander -
Michal Privoznik -
Peter Krempa