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

Re: [libvirt] [PATCH] Cancel migration/dump if user presses Ctrl-C when migration/dump is in progress



On Thu, Jan 06, 2011 at 05:16:53PM +0800, Hu Tao wrote:
> While migration/dump is in progress and virsh is waiting for its
> completion, user may want to terminate the progress by pressing
> Ctrl-C. But virsh just exits on user's Ctrl-C leaving migration/dump
> in background that user isn't even aware of. It's not reasonable.
> 
> This patch changes the behaviour for migration/dump. For other
> commands Ctrl-C still terminates virsh itself.
> ---
> Hi Daniel, Eric,
> 
> This patch is entirely different from my previous implementation so
> not titled with v2. It's simpler than the previous one and introduces
> less changes: not introducing threadpool in virsh;Ctrl-C remains
> terminating virsh if no job in progress.
> 
> Thanks for your review of the previous patch.
> 
>  src/remote/remote_driver.c |    9 +++++++--
>  tools/virsh.c              |   37 +++++++++++++++++++++++++++++++++++--
>  2 files changed, 42 insertions(+), 4 deletions(-)
> 
> diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
> index ee2de4a..59ec486 100644
> --- a/src/remote/remote_driver.c
> +++ b/src/remote/remote_driver.c
> @@ -9810,7 +9810,7 @@ processCallDispatchReply(virConnectPtr conn ATTRIBUTE_UNUSED,
>          remoteError(VIR_ERR_RPC,
>                      _("no call waiting for reply with serial %d"),
>                      hdr->serial);
> -        return -1;
> +        return -2;
>      }
>  
>      if (hdr->proc != thecall->proc_nr) {
> @@ -10160,7 +10160,12 @@ remoteIOEventLoop(virConnectPtr conn,
>          }
>  
>          if (fds[0].revents & POLLIN) {
> -            if (remoteIOHandleInput(conn, priv, flags) < 0)
> +            int ret = remoteIOHandleInput(conn, priv, flags);
> +            if (ret == -2) {
> +                /* Not expected result, repoll */
> +                remoteDriverUnlock(priv);
> +                goto repoll;
> +            } else if (ret < 0)
>                  goto error;
>          }
>  

I don't think this change is correct. The error scenario shown
in the first chunk is one that is hit when you have malformed
data on the wire. As such I don't believe it is safe to treat
this as a non-fatal recoverable error. Hitting Ctrl-C may
well cause this path to behave as you desire, but there are
other non-Ctrl-C based scenarios in which is it incorrect.

While I think the threadpool stuff was overkill, we do still
need 1 extra thread and I think the solution / patch overlaps
with the solution/patch proposed by Wen Congyang a few days
ago.

eg, we can create a setup that looks like this which I think
will solve both your own & Wen's needs for migration in
virsh.

 * Thread a  (Runs the migration)

      pthread_sigmask(mask with SIGINT blocked)
      virDomainMigrate()
      pthread_sigmask(original mask)


 * Thread b  (Monitors/controls the migration)

     gettimeofday(start)
     while (1) {
        gettimeofday(now)
        if (now - start) > timeout
            force guest to offline migrate
        if intCatched == TRUE
            abort migrate
        if --verbose
            pthread_sigmask(mask with SIGINT blocked)
            virDomainGetJobInfo()
            pthread_sigmask(original mask)
            print out progress info on console
     }


Since thread a blocks Ctrl-C, the signal will get
delivered to thread b instead, possibly delayed a
little if in the (short) virDomainGetJobInfo call.
Thus we don't need any changes to remote_driver.c
at all, and virsh can handle Ctrl-C on its own.

> diff --git a/tools/virsh.c b/tools/virsh.c
> index 55e2a68..e4d431e 100644
> --- a/tools/virsh.c
> +++ b/tools/virsh.c
> @@ -492,6 +492,15 @@ out:
>      last_error = NULL;
>  }
>  
> +static bool intCatched = FALSE;
> +
> +static void vshCatchInt(int sig ATTRIBUTE_UNUSED,
> +                        siginfo_t *siginfo ATTRIBUTE_UNUSED,
> +                        void *context ATTRIBUTE_UNUSED)
> +{
> +    intCatched = TRUE;
> +}
> +
>  /*
>   * Detection of disconnections and automatic reconnection support
>   */
> @@ -1838,6 +1847,9 @@ cmdDump(vshControl *ctl, const vshCmd *cmd)
>      char *to;
>      int ret = TRUE;
>      int flags = 0;
> +    int result;
> +    struct sigaction sig_action;
> +    struct sigaction old_sig_action;
>  
>      if (!vshConnectionUsability(ctl, ctl->conn))
>          return FALSE;
> @@ -1848,18 +1860,27 @@ cmdDump(vshControl *ctl, const vshCmd *cmd)
>      if (!(dom = vshCommandOptDomain(ctl, cmd, &name)))
>          return FALSE;
>  
> +    sig_action.sa_sigaction = vshCatchInt;
> +    sig_action.sa_flags = SA_SIGINFO;
> +    sigemptyset(&sig_action.sa_mask);
> +    sigaction(SIGINT, &sig_action, &old_sig_action);
> +
>      if (vshCommandOptBool (cmd, "live"))
>          flags |= VIR_DUMP_LIVE;
>      if (vshCommandOptBool (cmd, "crash"))
>          flags |= VIR_DUMP_CRASH;
>  
> -    if (virDomainCoreDump(dom, to, flags) == 0) {
> +    result = virDomainCoreDump(dom, to, flags);
> +    if (result == 0) {
>          vshPrint(ctl, _("Domain %s dumped to %s\n"), name, to);
> +    } else if (intCatched) {
> +        virDomainAbortJob(dom);
>      } else {
>          vshError(ctl, _("Failed to core dump domain %s to %s"), name, to);
>          ret = FALSE;
>      }
>  
> +    sigaction(SIGINT, &old_sig_action, NULL);
>      virDomainFree(dom);
>      return ret;
>  }
> @@ -3388,6 +3409,9 @@ cmdMigrate (vshControl *ctl, const vshCmd *cmd)
>      const char *desturi;
>      const char *migrateuri;
>      const char *dname;
> +    int result;
> +    struct sigaction sig_action;
> +    struct sigaction old_sig_action;
>      int flags = 0, found, ret = FALSE;
>  
>      if (!vshConnectionUsability (ctl, ctl->conn))
> @@ -3396,6 +3420,11 @@ cmdMigrate (vshControl *ctl, const vshCmd *cmd)
>      if (!(dom = vshCommandOptDomain (ctl, cmd, NULL)))
>          return FALSE;
>  
> +    sig_action.sa_sigaction = vshCatchInt;
> +    sig_action.sa_flags = SA_SIGINFO;
> +    sigemptyset(&sig_action.sa_mask);
> +    sigaction(SIGINT, &sig_action, &old_sig_action);
> +
>      desturi = vshCommandOptString (cmd, "desturi", &found);
>      if (!found)
>          goto done;
> @@ -3437,6 +3466,8 @@ cmdMigrate (vshControl *ctl, const vshCmd *cmd)
>  
>          if (virDomainMigrateToURI (dom, desturi, flags, dname, 0) == 0)
>              ret = TRUE;
> +        else if (intCatched)
> +            virDomainAbortJob(dom);
>      } else {
>          /* For traditional live migration, connect to the destination host directly. */
>          virConnectPtr dconn = NULL;
> @@ -3449,11 +3480,13 @@ cmdMigrate (vshControl *ctl, const vshCmd *cmd)
>          if (ddom) {
>              virDomainFree(ddom);
>              ret = TRUE;
> -        }
> +        } else if (intCatched)
> +            virDomainAbortJob(dom);
>          virConnectClose (dconn);
>      }
>  
>   done:
> +    sigaction(SIGINT, &old_sig_action, NULL);
>      if (dom) virDomainFree (dom);
>      return ret;
>  }


Regards,
Daniel


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