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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
