Re: [libvirt PATCH v3 1/7] tools: rewrite interactive job monitoring logic

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&gt; > --- > 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&gt;