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

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(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] Oh, I already fixed that, which means it is probably squashed into one of the later patches in this series :-(