[libvirt] [PATCH 0/3] virsh: Handle interrupting of jobs manually
Using Ctrl+C to abort migration has a side effect of killing ssh transports used to execute the migration. Add manual handling to avoid this issue. Peter Krempa (3): virsh-domain: rename print_job_progress to vshPrintJobProgress virsh: Remember terminal state when starting and add helpers virsh-domain: Avoid killing ssh transport tunnels when cancelling job tools/virsh-domain.c | 65 +++++++++++++++++++++++++++++++++------------------- tools/virsh.c | 54 +++++++++++++++++++++++++++++++++++++++++++ tools/virsh.h | 9 ++++++++ 3 files changed, 105 insertions(+), 23 deletions(-) -- 1.8.3.2
--- tools/virsh-domain.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 83b9c3f..3fd57fd 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -1452,8 +1452,8 @@ cleanup: } static void -print_job_progress(const char *label, unsigned long long remaining, - unsigned long long total) +vshPrintJobProgress(const char *label, unsigned long long remaining, + unsigned long long total) { int progress; @@ -1624,8 +1624,8 @@ cmdBlockCommit(vshControl *ctl, const vshCmd *cmd) break; if (verbose) - print_job_progress(_("Block Commit"), - info.end - info.cur, info.end); + vshPrintJobProgress(_("Block Commit"), + info.end - info.cur, info.end); GETTIMEOFDAY(&curr); if (intCaught || (timeout && @@ -1650,7 +1650,7 @@ cmdBlockCommit(vshControl *ctl, const vshCmd *cmd) if (verbose && !quit) { /* printf [100 %] */ - print_job_progress(_("Block Commit"), 0, 1); + vshPrintJobProgress(_("Block Commit"), 0, 1); } vshPrint(ctl, "\n%s", quit ? _("Commit aborted") : _("Commit complete")); @@ -1818,7 +1818,7 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd) break; if (verbose) - print_job_progress(_("Block Copy"), info.end - info.cur, info.end); + vshPrintJobProgress(_("Block Copy"), info.end - info.cur, info.end); if (info.cur == info.end) break; @@ -1961,7 +1961,7 @@ cmdBlockJob(vshControl *ctl, const vshCmd *cmd) break; } - print_job_progress(type, info.end - info.cur, info.end); + vshPrintJobProgress(type, info.end - info.cur, info.end); if (info.bandwidth != 0) vshPrint(ctl, _(" Bandwidth limit: %lu MiB/s\n"), info.bandwidth); return true; @@ -2095,7 +2095,7 @@ cmdBlockPull(vshControl *ctl, const vshCmd *cmd) break; if (verbose) - print_job_progress(_("Block Pull"), info.end - info.cur, info.end); + vshPrintJobProgress(_("Block Pull"), info.end - info.cur, info.end); GETTIMEOFDAY(&curr); if (intCaught || (timeout && @@ -2120,7 +2120,7 @@ cmdBlockPull(vshControl *ctl, const vshCmd *cmd) if (verbose && !quit) { /* printf [100 %] */ - print_job_progress(_("Block Pull"), 0, 1); + vshPrintJobProgress(_("Block Pull"), 0, 1); } vshPrint(ctl, "\n%s", quit ? _("Pull aborted") : _("Pull complete")); @@ -3553,7 +3553,7 @@ repoll: retchar == '0') { if (verbose) { /* print [100 %] */ - print_job_progress(label, 0, 1); + vshPrintJobProgress(label, 0, 1); } break; } @@ -3588,8 +3588,8 @@ repoll: ret = virDomainGetJobInfo(dom, &jobinfo); pthread_sigmask(SIG_SETMASK, &oldsigmask, NULL); if (ret == 0) - print_job_progress(label, jobinfo.dataRemaining, - jobinfo.dataTotal); + vshPrintJobProgress(label, jobinfo.dataRemaining, + jobinfo.dataTotal); } } -- 1.8.3.2
On 08/29/2013 09:52 AM, Peter Krempa wrote:
--- tools/virsh-domain.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-)
ACK, mechanical and safe for freeze. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 08/29/13 19:20, Eric Blake wrote:
On 08/29/2013 09:52 AM, Peter Krempa wrote:
--- tools/virsh-domain.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-)
ACK, mechanical and safe for freeze.
I've pushed this one. Thanks. Peter
This patch adds instrumentation to allow modification of config of the terminal in virsh and successful reset of the state afterwards. The added helpers allow to disable receiving of SIGINT when pressing the key sequence (Ctrl+C usualy). This normally sends SIGINT to the foreground process group which kills ssh processes used for transport of the data. --- The termios code may need protecting with #ifndef WIN32. tools/virsh.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ tools/virsh.h | 9 +++++++++ 2 files changed, 63 insertions(+) diff --git a/tools/virsh.c b/tools/virsh.c index 38345c0..2f04e6a 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -2213,6 +2213,53 @@ vshPrintExtra(vshControl *ctl, const char *format, ...) } +bool +vshTTYIsInterruptCharacter(vshControl *ctl, + const char chr) +{ + if (ctl->istty && + ctl->termattr.c_cc[VINTR] == chr) + return true; + + return false; +} + + +int +vshTTYDisableInterrupt(vshControl *ctl) +{ + struct termios termset = ctl->termattr; + + if (!ctl->istty) + return -1; + + /* check if we need to set the terminal */ + if (termset.c_cc[VINTR] == _POSIX_VDISABLE) + return 0; + + termset.c_cc[VINTR] = _POSIX_VDISABLE; + termset.c_lflag &= ~ICANON; + + if (tcsetattr(STDIN_FILENO, TCSANOW, &termset) < 0) + return -1; + + return 0; +} + + +int +vshTTYRestore(vshControl *ctl) +{ + if (!ctl->istty) + return 0; + + if (tcsetattr(STDIN_FILENO, TCSAFLUSH, &ctl->termattr) < 0) + return -1; + + return 0; +} + + void vshError(vshControl *ctl, const char *format, ...) { @@ -3157,6 +3204,13 @@ main(int argc, char **argv) return EXIT_FAILURE; } + if (isatty(STDIN_FILENO)) { + ctl->istty = true; + + if (tcgetattr(STDIN_FILENO, &ctl->termattr) < 0) + ctl->istty = false; + } + if (virMutexInit(&ctl->lock) < 0) { vshError(ctl, "%s", _("Failed to initialize mutex")); return EXIT_FAILURE; diff --git a/tools/virsh.h b/tools/virsh.h index 570d6a9..db5934f 100644 --- a/tools/virsh.h +++ b/tools/virsh.h @@ -32,6 +32,7 @@ # include <unistd.h> # include <sys/stat.h> # include <inttypes.h> +# include <termios.h> # include "internal.h" # include "virerror.h" @@ -240,6 +241,9 @@ struct _vshControl { const char *escapeChar; /* String representation of console escape character */ + + struct termios termattr; /* settings of the tty terminal */ + bool istty; /* is the terminal a tty */ }; struct _vshCmdGrp { @@ -350,6 +354,11 @@ void vshReportError(vshControl *ctl); void vshResetLibvirtError(void); void vshSaveLibvirtError(void); +/* terminal modifications */ +bool vshTTYIsInterruptCharacter(vshControl *ctl, const char chr); +int vshTTYDisableInterrupt(vshControl *ctl); +int vshTTYRestore(vshControl *ctl); + /* allocation wrappers */ void *_vshMalloc(vshControl *ctl, size_t sz, const char *filename, int line); # define vshMalloc(_ctl, _sz) _vshMalloc(_ctl, _sz, __FILE__, __LINE__) -- 1.8.3.2
On 29.08.2013 17:52, Peter Krempa wrote:
This patch adds instrumentation to allow modification of config of the terminal in virsh and successful reset of the state afterwards.
The added helpers allow to disable receiving of SIGINT when pressing the key sequence (Ctrl+C usualy). This normally sends SIGINT to the foreground process group which kills ssh processes used for transport of the data. --- The termios code may need protecting with #ifndef WIN32.
tools/virsh.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ tools/virsh.h | 9 +++++++++ 2 files changed, 63 insertions(+)
ACK Michal
The vshWatchJob function registers a SIGINT handler that is used to abort the active job and does not terminate virsh. Unfortunately, this breaks when using the ssh transport as SIGINT is sent to the foreground process group including the ssh transport processes which terminate. This breaks the connection and migration is left in a insane state. With this patch the terminal is modified to ignore key binding that sends SIGINT and does the handling manually. Resoves: https://bugzilla.redhat.com/show_bug.cgi?id=983348 --- tools/virsh-domain.c | 45 ++++++++++++++++++++++++++++++++------------- 1 file changed, 32 insertions(+), 13 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 3fd57fd..c87cf6a 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -3522,13 +3522,15 @@ vshWatchJob(vshControl *ctl, { struct sigaction sig_action; struct sigaction old_sig_action; - struct pollfd pollfd; + struct pollfd pollfd[2] = {{.fd = pipe_fd, .events = POLLIN, .revents = 0}, + {.fd = STDIN_FILENO, .events = POLLIN, .revents = 0}}; struct timeval start, curr; virDomainJobInfo jobinfo; int ret = -1; char retchar; bool functionReturn = false; sigset_t sigmask, oldsigmask; + bool jobStarted = false; sigemptyset(&sigmask); sigaddset(&sigmask, SIGINT); @@ -3539,16 +3541,21 @@ vshWatchJob(vshControl *ctl, sigemptyset(&sig_action.sa_mask); sigaction(SIGINT, &sig_action, &old_sig_action); - pollfd.fd = pipe_fd; - pollfd.events = POLLIN; - pollfd.revents = 0; - GETTIMEOFDAY(&start); while (1) { -repoll: - ret = poll(&pollfd, 1, 500); + ret = poll((struct pollfd *)&pollfd, 2, 500); if (ret > 0) { - if (pollfd.revents & POLLIN && + if (pollfd[1].revents & POLLIN && + saferead(STDIN_FILENO, &retchar, sizeof(retchar)) > 0) { + if (vshTTYIsInterruptCharacter(ctl, retchar)) { + virDomainAbortJob(dom); + goto cleanup; + } else { + continue; + } + } + + if (pollfd[0].revents & POLLIN && saferead(pipe_fd, &retchar, sizeof(retchar)) > 0 && retchar == '0') { if (verbose) { @@ -3566,7 +3573,7 @@ repoll: virDomainAbortJob(dom); intCaught = 0; } else { - goto repoll; + continue; } } goto cleanup; @@ -3583,13 +3590,24 @@ repoll: timeout = 0; } - if (verbose) { + if (verbose || !jobStarted) { pthread_sigmask(SIG_BLOCK, &sigmask, &oldsigmask); ret = virDomainGetJobInfo(dom, &jobinfo); pthread_sigmask(SIG_SETMASK, &oldsigmask, NULL); - if (ret == 0) - vshPrintJobProgress(label, jobinfo.dataRemaining, - jobinfo.dataTotal); + if (ret == 0) { + if (verbose) + vshPrintJobProgress(label, jobinfo.dataRemaining, + jobinfo.dataTotal); + + if (!jobStarted && + (jobinfo.type == VIR_DOMAIN_JOB_BOUNDED || + jobinfo.type == VIR_DOMAIN_JOB_UNBOUNDED)) { + vshTTYDisableInterrupt(ctl); + jobStarted = true; + } + } else { + vshResetLibvirtError(); + } } } @@ -3597,6 +3615,7 @@ repoll: cleanup: sigaction(SIGINT, &old_sig_action, NULL); + vshTTYRestore(ctl); return functionReturn; } -- 1.8.3.2
On 29.08.2013 17:52, Peter Krempa wrote:
The vshWatchJob function registers a SIGINT handler that is used to abort the active job and does not terminate virsh. Unfortunately, this breaks when using the ssh transport as SIGINT is sent to the foreground process group including the ssh transport processes which terminate. This breaks the connection and migration is left in a insane state.
With this patch the terminal is modified to ignore key binding that sends SIGINT and does the handling manually.
Resoves: https://bugzilla.redhat.com/show_bug.cgi?id=983348 --- tools/virsh-domain.c | 45 ++++++++++++++++++++++++++++++++------------- 1 file changed, 32 insertions(+), 13 deletions(-)
ACK Michal
On 09/03/13 09:53, Michal Privoznik wrote:
On 29.08.2013 17:52, Peter Krempa wrote:
The vshWatchJob function registers a SIGINT handler that is used to abort the active job and does not terminate virsh. Unfortunately, this breaks when using the ssh transport as SIGINT is sent to the foreground process group including the ssh transport processes which terminate. This breaks the connection and migration is left in a insane state.
With this patch the terminal is modified to ignore key binding that sends SIGINT and does the handling manually.
Resoves: https://bugzilla.redhat.com/show_bug.cgi?id=983348 --- tools/virsh-domain.c | 45 ++++++++++++++++++++++++++++++++------------- 1 file changed, 32 insertions(+), 13 deletions(-)
ACK
Michal
Thanks for the reviews, I've pushed 2/3 and 3/3 now. Peter
participants (3)
-
Eric Blake -
Michal Privoznik -
Peter Krempa