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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx>
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 :|
