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

[Pavel Hrdina]

On Thu, Feb 06, 2020 at 02:14:47PM +0000, Daniel P. Berrangé wrote:
> 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 at 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 :-(

I guess you mean that you fixed that locally because the patch series
here on list doesn't have that fix in any of the following patches.

Would be nice to move it to correct patch before pushing. :)

Pavel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20200206/32bf8407/attachment-0001.sig>