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(a)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]
+
+ 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(a)redhat.com>