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]
> +
> + 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>
Attachment:
signature.asc
Description: PGP signature
