On Thu, Feb 06, 2020 at 03:06:53PM +0100, Pavel Hrdina wrote:
On Wed, Feb 05, 2020 at 05:18:52PM +0000, Daniel P. Berrangé wrote:
For long running jobs (save, managed save, dump & live migrate) virsh runs a background thread for executing the job and then has the main thread catch Ctrl-C for graceful shutdown, as well as displaying progress info.
The monitoring code is written using poll, with a pipe used to get the completion status from the thread. Using a pipe and poll is problematic for Windows portability. This rewrites the code to use a GMainLoop instance for monitoring stdin and doing progress updates. The use of a pipe is entirely eliminated, instead there is just a shared variable between both threads containing the job completion status.
No mutex locking is used because the background thread writes to the variable only when the main loop is still running, while the foreground thread only reads it after the main loop has exited.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- tools/virsh-domain.c | 388 +++++++++++++++++++++++++------------------ tools/virsh.h | 3 +- 2 files changed, 232 insertions(+), 159 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 781463f0e2..bf828c90c4 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -23,7 +23,6 @@ #include "virsh-util.h"
#include <fcntl.h> -#include <poll.h> #include <signal.h> #include <sys/time.h>
@@ -4224,7 +4223,6 @@ doSave(void *opaque) virshCtrlData *data = opaque; vshControl *ctl = data->ctl; const vshCmd *cmd = data->cmd; - char ret = '1'; virDomainPtr dom = NULL; const char *name = NULL; const char *to = NULL; @@ -4269,7 +4267,7 @@ doSave(void *opaque) goto out; }
- ret = '0'; + data->ret = 0;
out: #ifndef WIN32 @@ -4278,18 +4276,126 @@ doSave(void *opaque) #endif /* !WIN32 */ virshDomainFree(dom); VIR_FREE(xml); - ignore_value(safewrite(data->writefd, &ret, sizeof(ret))); + g_main_loop_quit(data->eventLoop); }
typedef void (*jobWatchTimeoutFunc)(vshControl *ctl, virDomainPtr dom, void *opaque);
-static bool +struct virshWatchData { + vshControl *ctl; + virDomainPtr dom; + jobWatchTimeoutFunc timeout_func; + void *opaque; + const char *label; + GIOChannel *stdin_ioc; + bool jobStarted; + bool verbose; +}; + +static gboolean +virshWatchTimeout(gpointer opaque) +{ + struct virshWatchData *data = opaque; + + /* suspend the domain when migration timeouts. */ + vshDebug(data->ctl, VSH_ERR_DEBUG, "watchJob: timeout\n"); + if (data->timeout_func) + (data->timeout_func)(data->ctl, data->dom, data->opaque); + + return G_SOURCE_REMOVE; +} + + +static gboolean +virshWatchProgress(gpointer opaque) +{ + struct virshWatchData *data = opaque; + virDomainJobInfo jobinfo; +#ifndef WIN32 + sigset_t sigmask, oldsigmask; + int ret;
The `ret` variable needs to be declared outside of the #ifndef block becuase [1]
Oh, I already fixed that, which means it is probably squashed into one of the later patches in this series :-(
+ + sigemptyset(&sigmask); + sigaddset(&sigmask, SIGINT); + + vshDebug(data->ctl, VSH_ERR_DEBUG, "%s", + "watchJob: progress update\n");
This debug message should be outside of the block as well.
+ pthread_sigmask(SIG_BLOCK, &sigmask, &oldsigmask); +#endif /* !WIN32 */ + ret = virDomainGetJobInfo(data->dom, &jobinfo);
[1] here it would fail for mingw builds.
Otherwise looks good so with the issues fixed:
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|