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

[Pavel Hrdina]

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]

> +
> +    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 at redhat.com>
-------------- 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/1af7269b/attachment-0001.sig>