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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux