[libvirt] [PATCH 0/4] virsh: Make more commands cancelable via ^C

Currently, only migration command allows job progress reporting and cancel via SIGINT. However, there are more job oriented commands which take long time to finish and user might want to cancel them like he can do in migration. Such commands are: dump, save, managedsave. Michal Privoznik (4): virsh: Move job watch code to a separate function virsh: Use do_job_watch in cmdDump virsh: Use do_job_watch in cmdSave virsh: Use do_job_watch in cmdManagedSave tools/virsh.c | 425 +++++++++++++++++++++++++++++++++++++++++-------------- tools/virsh.pod | 24 ++-- 2 files changed, 330 insertions(+), 119 deletions(-) -- 1.7.3.4

called do_watch_job. This can be later used in other job oriented commands like dump, save, managedsave to report progress and allow user to cancel via ^C. --- tools/virsh.c | 187 ++++++++++++++++++++++++++++++++++---------------------- 1 files changed, 113 insertions(+), 74 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 583ec6d..5f9a9ff 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -394,6 +394,27 @@ static char *editWriteToTempFile (vshControl *ctl, const char *doc); static int editFile (vshControl *ctl, const char *filename); static char *editReadBackFile (vshControl *ctl, const char *filename); +/* Typedefs, function prototypes for job progress reporting. + * There are used by some long lingering commands like + * migrate, dump, save, managedsave. + */ +typedef struct __vshCtrlData { + vshControl *ctl; + const vshCmd *cmd; + int writefd; +} vshCtrlData; + +typedef void (*jobWatchTimeoutFunc) (vshControl *ctl, virDomainPtr dom); + +static bool +do_watch_job(vshControl *ctl, + virDomainPtr dom, + bool verbose, + int pipe_fd, + int timeout, + jobWatchTimeoutFunc timeout_func, + const char *label); + static void *_vshMalloc(vshControl *ctl, size_t sz, const char *filename, int line); #define vshMalloc(_ctl, _sz) _vshMalloc(_ctl, _sz, __FILE__, __LINE__) @@ -5720,12 +5741,6 @@ static const vshCmdOptDef opts_migrate[] = { {NULL, 0, 0, NULL} }; -typedef struct __vshCtrlData { - vshControl *ctl; - const vshCmd *cmd; - int writefd; -} vshCtrlData; - static void doMigrate (void *opaque) { @@ -5858,75 +5873,44 @@ print_job_progress(const char *label, unsigned long long remaining, fflush(stderr); } +static void +on_migration_timeout(vshControl *ctl, + virDomainPtr dom) +{ + vshDebug(ctl, VSH_ERR_DEBUG, "suspend the domain " + "when migration timeouts\n"); + virDomainSuspend(dom); +} + static bool -cmdMigrate (vshControl *ctl, const vshCmd *cmd) +do_watch_job(vshControl *ctl, + virDomainPtr dom, + bool verbose, + int pipe_fd, + int timeout, + jobWatchTimeoutFunc timeout_func, + const char *label) { - virDomainPtr dom = NULL; - int p[2] = {-1, -1}; - int ret = -1; - bool functionReturn = false; - virThread workerThread; - struct pollfd pollfd; - char retchar; struct sigaction sig_action; struct sigaction old_sig_action; - virDomainJobInfo jobinfo; - bool verbose = false; - int timeout = 0; + struct pollfd pollfd; struct timeval start, curr; - bool live_flag = false; - vshCtrlData data; + virDomainJobInfo jobinfo; + int ret = -1; + char retchar; + bool functionReturn = false; sigset_t sigmask, oldsigmask; sigemptyset(&sigmask); sigaddset(&sigmask, SIGINT); - if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) - return false; - - if (vshCommandOptBool (cmd, "verbose")) - verbose = true; - - if (vshCommandOptBool (cmd, "live")) - live_flag = true; - if (vshCommandOptInt(cmd, "timeout", &timeout) > 0) { - if (! live_flag) { - vshError(ctl, "%s", _("migrate: Unexpected timeout for offline migration")); - goto cleanup; - } - - if (timeout < 1) { - vshError(ctl, "%s", _("migrate: Invalid timeout")); - goto cleanup; - } - - /* Ensure that we can multiply by 1000 without overflowing. */ - if (timeout > INT_MAX / 1000) { - vshError(ctl, "%s", _("migrate: Timeout is too big")); - goto cleanup; - } - } - - if (pipe(p) < 0) - goto cleanup; - - data.ctl = ctl; - data.cmd = cmd; - data.writefd = p[1]; - - if (virThreadCreate(&workerThread, - true, - doMigrate, - &data) < 0) - goto cleanup; - intCaught = 0; sig_action.sa_sigaction = vshCatchInt; sig_action.sa_flags = SA_SIGINFO; sigemptyset(&sig_action.sa_mask); sigaction(SIGINT, &sig_action, &old_sig_action); - pollfd.fd = p[0]; + pollfd.fd = pipe_fd; pollfd.events = POLLIN; pollfd.revents = 0; @@ -5935,18 +5919,16 @@ cmdMigrate (vshControl *ctl, const vshCmd *cmd) repoll: ret = poll(&pollfd, 1, 500); if (ret > 0) { - if (saferead(p[0], &retchar, sizeof(retchar)) > 0) { - if (retchar == '0') { - functionReturn = true; + if (pollfd.revents & POLLIN && + saferead(pipe_fd, &retchar, sizeof(retchar)) > 0 && + retchar == '0') { if (verbose) { /* print [100 %] */ - print_job_progress("Migration", 0, 1); + print_job_progress(label, 0, 1); } - } else - functionReturn = false; - } else - functionReturn = false; - break; + break; + } + goto cleanup; } if (ret < 0) { @@ -5957,17 +5939,16 @@ repoll: } else goto repoll; } - functionReturn = false; - break; + goto cleanup; } GETTIMEOFDAY(&curr); if ( timeout && ((int)(curr.tv_sec - start.tv_sec) * 1000 + \ (int)(curr.tv_usec - start.tv_usec) / 1000) > timeout * 1000 ) { /* suspend the domain when migration timeouts. */ - vshDebug(ctl, VSH_ERR_DEBUG, - "suspend the domain when migration timeouts\n"); - virDomainSuspend(dom); + vshDebug(ctl, VSH_ERR_DEBUG, "%s timeout", label); + if (timeout_func) + (timeout_func)(ctl, dom); timeout = 0; } @@ -5976,12 +5957,70 @@ repoll: ret = virDomainGetJobInfo(dom, &jobinfo); pthread_sigmask(SIG_SETMASK, &oldsigmask, NULL); if (ret == 0) - print_job_progress("Migration", jobinfo.dataRemaining, + print_job_progress(label, jobinfo.dataRemaining, jobinfo.dataTotal); } } + functionReturn = true; + +cleanup: sigaction(SIGINT, &old_sig_action, NULL); + return functionReturn; +} + +static bool +cmdMigrate (vshControl *ctl, const vshCmd *cmd) +{ + virDomainPtr dom = NULL; + int p[2] = {-1, -1}; + virThread workerThread; + bool verbose = false; + bool functionReturn = false; + int timeout = 0; + bool live_flag = false; + vshCtrlData data; + + if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) + return false; + + if (vshCommandOptBool (cmd, "verbose")) + verbose = true; + + if (vshCommandOptBool (cmd, "live")) + live_flag = true; + if (vshCommandOptInt(cmd, "timeout", &timeout) > 0) { + if (! live_flag) { + vshError(ctl, "%s", _("migrate: Unexpected timeout for offline migration")); + goto cleanup; + } + + if (timeout < 1) { + vshError(ctl, "%s", _("migrate: Invalid timeout")); + goto cleanup; + } + + /* Ensure that we can multiply by 1000 without overflowing. */ + if (timeout > INT_MAX / 1000) { + vshError(ctl, "%s", _("migrate: Timeout is too big")); + goto cleanup; + } + } + + if (pipe(p) < 0) + goto cleanup; + + data.ctl = ctl; + data.cmd = cmd; + data.writefd = p[1]; + + if (virThreadCreate(&workerThread, + true, + doMigrate, + &data) < 0) + goto cleanup; + functionReturn = do_watch_job(ctl, dom, verbose, p[0], timeout, + on_migration_timeout, "Migration"); virThreadJoin(&workerThread); -- 1.7.3.4

On 12/20/2011 08:21 AM, Michal Privoznik wrote:
called do_watch_job. This can be later used in other job oriented commands like dump, save, managedsave to report progress and allow user to cancel via ^C. --- tools/virsh.c | 187 ++++++++++++++++++++++++++++++++++---------------------- 1 files changed, 113 insertions(+), 74 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index 583ec6d..5f9a9ff 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -394,6 +394,27 @@ static char *editWriteToTempFile (vshControl *ctl, const char *doc); static int editFile (vshControl *ctl, const char *filename); static char *editReadBackFile (vshControl *ctl, const char *filename);
+/* Typedefs, function prototypes for job progress reporting. + * There are used by some long lingering commands like + * migrate, dump, save, managedsave. + */ +typedef struct __vshCtrlData {
Single underscore is sufficient; but then again this is just code motion of existing code.
+ vshControl *ctl; + const vshCmd *cmd; + int writefd; +} vshCtrlData; + +typedef void (*jobWatchTimeoutFunc) (vshControl *ctl, virDomainPtr dom);
Should this also have a 'void *opaque' parameter, so future expansions can use it if needed?
+ +static bool +do_watch_job(vshControl *ctl,
I would have named this vshWatchJob (we don't have very many function names with underscores, preferring camelCase instead), but that's minor. I can live with either name.
+ virDomainPtr dom, + bool verbose, + int pipe_fd, + int timeout, + jobWatchTimeoutFunc timeout_func,
should this be accompanied with a 'void *opaque' to pass to timeout_func (you can pass NULL for now)?
+ const char *label); + static void *_vshMalloc(vshControl *ctl, size_t sz, const char *filename, int line); #define vshMalloc(_ctl, _sz) _vshMalloc(_ctl, _sz, __FILE__, __LINE__)
@@ -5720,12 +5741,6 @@ static const vshCmdOptDef opts_migrate[] = { {NULL, 0, 0, NULL} };
-typedef struct __vshCtrlData { - vshControl *ctl; - const vshCmd *cmd; - int writefd; -} vshCtrlData; - static void doMigrate (void *opaque) { @@ -5858,75 +5873,44 @@ print_job_progress(const char *label, unsigned long long remaining, fflush(stderr); }
+static void +on_migration_timeout(vshControl *ctl, + virDomainPtr dom)
I would have named this vshMigrationTimeout.
+{ + vshDebug(ctl, VSH_ERR_DEBUG, "suspend the domain " + "when migration timeouts\n");
Here, I would use: "suspending the domain, since migration timed out"
@@ -5935,18 +5919,16 @@ cmdMigrate (vshControl *ctl, const vshCmd *cmd) repoll: ret = poll(&pollfd, 1, 500); if (ret > 0) { - if (saferead(p[0], &retchar, sizeof(retchar)) > 0) { - if (retchar == '0') { - functionReturn = true; + if (pollfd.revents & POLLIN && + saferead(pipe_fd, &retchar, sizeof(retchar)) > 0 && + retchar == '0') { if (verbose) { /* print [100 %] */ - print_job_progress("Migration", 0, 1);
Hmm - we weren't translating this string previously.
+ + if (virThreadCreate(&workerThread, + true, + doMigrate, + &data) < 0) + goto cleanup; + functionReturn = do_watch_job(ctl, dom, verbose, p[0], timeout, + on_migration_timeout, "Migration");
The last parameter should be _("Migration"). Overall, looks reasonable. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 21.12.2011 18:58, Eric Blake wrote:
On 12/20/2011 08:21 AM, Michal Privoznik wrote:
called do_watch_job. This can be later used in other job oriented commands like dump, save, managedsave to report progress and allow user to cancel via ^C. --- tools/virsh.c | 187 ++++++++++++++++++++++++++++++++++---------------------- 1 files changed, 113 insertions(+), 74 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index 583ec6d..5f9a9ff 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -394,6 +394,27 @@ static char *editWriteToTempFile (vshControl *ctl, const char *doc); static int editFile (vshControl *ctl, const char *filename); static char *editReadBackFile (vshControl *ctl, const char *filename);
+/* Typedefs, function prototypes for job progress reporting. + * There are used by some long lingering commands like + * migrate, dump, save, managedsave. + */ +typedef struct __vshCtrlData {
Single underscore is sufficient; but then again this is just code motion of existing code.
+ vshControl *ctl; + const vshCmd *cmd; + int writefd; +} vshCtrlData; + +typedef void (*jobWatchTimeoutFunc) (vshControl *ctl, virDomainPtr dom);
Should this also have a 'void *opaque' parameter, so future expansions can use it if needed?
+ +static bool +do_watch_job(vshControl *ctl,
I would have named this vshWatchJob (we don't have very many function names with underscores, preferring camelCase instead), but that's minor. I can live with either name.
+ virDomainPtr dom, + bool verbose, + int pipe_fd, + int timeout, + jobWatchTimeoutFunc timeout_func,
should this be accompanied with a 'void *opaque' to pass to timeout_func (you can pass NULL for now)?
+ const char *label); + static void *_vshMalloc(vshControl *ctl, size_t sz, const char *filename, int line); #define vshMalloc(_ctl, _sz) _vshMalloc(_ctl, _sz, __FILE__, __LINE__)
@@ -5720,12 +5741,6 @@ static const vshCmdOptDef opts_migrate[] = { {NULL, 0, 0, NULL} };
-typedef struct __vshCtrlData { - vshControl *ctl; - const vshCmd *cmd; - int writefd; -} vshCtrlData; - static void doMigrate (void *opaque) { @@ -5858,75 +5873,44 @@ print_job_progress(const char *label, unsigned long long remaining, fflush(stderr); }
+static void +on_migration_timeout(vshControl *ctl, + virDomainPtr dom)
I would have named this vshMigrationTimeout.
+{ + vshDebug(ctl, VSH_ERR_DEBUG, "suspend the domain " + "when migration timeouts\n");
Here, I would use: "suspending the domain, since migration timed out"
@@ -5935,18 +5919,16 @@ cmdMigrate (vshControl *ctl, const vshCmd *cmd) repoll: ret = poll(&pollfd, 1, 500); if (ret > 0) { - if (saferead(p[0], &retchar, sizeof(retchar)) > 0) { - if (retchar == '0') { - functionReturn = true; + if (pollfd.revents & POLLIN && + saferead(pipe_fd, &retchar, sizeof(retchar)) > 0 && + retchar == '0') { if (verbose) { /* print [100 %] */ - print_job_progress("Migration", 0, 1);
Hmm - we weren't translating this string previously.
+ + if (virThreadCreate(&workerThread, + true, + doMigrate, + &data) < 0) + goto cleanup; + functionReturn = do_watch_job(ctl, dom, verbose, p[0], timeout, + on_migration_timeout, "Migration");
The last parameter should be _("Migration").
Overall, looks reasonable.
Fixed the nits and pushed. Thanks.

This patch alters dumping code, so we can report progress and allow cancel via ^C. --- tools/virsh.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++------- tools/virsh.pod | 8 +++--- 2 files changed, 73 insertions(+), 14 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 5f9a9ff..c2440e2 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -3089,26 +3089,36 @@ static const vshCmdOptDef opts_dump[] = { {"reset", VSH_OT_BOOL, 0, N_("reset the domain after core dump")}, {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, {"file", VSH_OT_DATA, VSH_OFLAG_REQ, N_("where to dump the core")}, + {"verbose", VSH_OT_BOOL, 0, N_("display the progress of dump")}, {NULL, 0, 0, NULL} }; -static bool -cmdDump(vshControl *ctl, const vshCmd *cmd) +static void +doDump(void *opaque) { - virDomainPtr dom; + char ret = '1'; + vshCtrlData *data = opaque; + vshControl *ctl = data->ctl; + const vshCmd *cmd = data->cmd; + virDomainPtr dom = NULL; + sigset_t sigmask, oldsigmask; const char *name = NULL; const char *to = NULL; - bool ret = false; unsigned int flags = 0; + sigemptyset(&sigmask); + sigaddset(&sigmask, SIGINT); + if (pthread_sigmask(SIG_BLOCK, &sigmask, &oldsigmask) < 0) + goto out_sig; + if (!vshConnectionUsability(ctl, ctl->conn)) - return false; + goto out; if (vshCommandOptString(cmd, "file", &to) <= 0) - return false; + goto out; if (!(dom = vshCommandOptDomain(ctl, cmd, &name))) - return false; + goto out; if (vshCommandOptBool (cmd, "live")) flags |= VIR_DUMP_LIVE; @@ -3121,14 +3131,63 @@ cmdDump(vshControl *ctl, const vshCmd *cmd) if (virDomainCoreDump(dom, to, flags) < 0) { vshError(ctl, _("Failed to core dump domain %s to %s"), name, to); - goto cleanup; + goto out; } - vshPrint(ctl, _("Domain %s dumped to %s\n"), name, to); - ret = true; + ret = '0'; +out: + pthread_sigmask(SIG_SETMASK, &oldsigmask, NULL); +out_sig: + if (dom) + virDomainFree (dom); + ignore_value(safewrite(data->writefd, &ret, sizeof(ret))); +} + +static bool +cmdDump(vshControl *ctl, const vshCmd *cmd) +{ + virDomainPtr dom; + int p[2] = { -1, -1}; + bool ret = false; + bool verbose = false; + const char *name = NULL; + const char *to = NULL; + vshCtrlData data; + virThread workerThread; + + if (!(dom = vshCommandOptDomain(ctl, cmd, &name))) + return false; + + if (vshCommandOptString(cmd, "file", &to) <= 0) + return false; + + if (vshCommandOptBool (cmd, "verbose")) + verbose = true; + + if (pipe(p) < 0) + goto cleanup; + + data.ctl = ctl; + data.cmd = cmd; + data.writefd = p[1]; + + if (virThreadCreate(&workerThread, + true, + doDump, + &data) < 0) + goto cleanup; + + ret = do_watch_job(ctl, dom, verbose, p[0], 0, NULL, "Dump"); + + virThreadJoin(&workerThread); + + if (ret) + vshPrint(ctl, _("\nDomain %s dumped to %s\n"), name, to); cleanup: virDomainFree(dom); + VIR_FORCE_CLOSE(p[0]); + VIR_FORCE_CLOSE(p[1]); return ret; } diff --git a/tools/virsh.pod b/tools/virsh.pod index dbe5165..5a5fbf8 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -678,7 +678,7 @@ Convert the file I<xml> in domain XML format to the native guest configuration format named by I<format>. =item B<dump> I<domain-id> I<corefilepath> [I<--bypass-cache>] -{ [I<--live>] | [I<--crash>] | [I<--reset>] } +{ [I<--live>] | [I<--crash>] | [I<--reset>] } [I<--verbose>] Dumps the core of a domain to a file for analysis. If I<--live> is specified, the domain continues to run until the core @@ -691,9 +691,9 @@ If I<--bypass-cache> is specified, the save will avoid the file system cache, although this may slow down the operation. The progress may be monitored using B<domjobinfo> virsh command and canceled -with B<domjobabort> command (sent by another virsh instance). Interrupting -(usually with C<Ctrl-C>) the virsh process which runs B<dump> command is not -enough to actually cancel the operation. +with B<domjobabort> command (sent by another virsh instance). Another option +is to send SIGINT (usually with C<Ctrl-C>) to the virsh process running +B<dump> command. I<--verbose> displays the progress of dump. NOTE: Some hypervisors may require the user to manually ensure proper permissions on file and path specified by argument I<corefilepath>. -- 1.7.3.4

On 12/20/2011 08:21 AM, Michal Privoznik wrote:
This patch alters dumping code, so we can report progress and allow cancel via ^C. --- tools/virsh.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++------- tools/virsh.pod | 8 +++--- 2 files changed, 73 insertions(+), 14 deletions(-)
+ sigemptyset(&sigmask); + sigaddset(&sigmask, SIGINT); + if (pthread_sigmask(SIG_BLOCK, &sigmask, &oldsigmask) < 0) + goto out_sig; + if (!vshConnectionUsability(ctl, ctl->conn)) - return false; + goto out;
Is it any more efficient to do the argument validation prior to the pthread_sigmask?
+ + ret = do_watch_job(ctl, dom, verbose, p[0], 0, NULL, "Dump");
_("Dump") ACK. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 21.12.2011 20:08, Eric Blake wrote:
On 12/20/2011 08:21 AM, Michal Privoznik wrote:
This patch alters dumping code, so we can report progress and allow cancel via ^C. --- tools/virsh.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++------- tools/virsh.pod | 8 +++--- 2 files changed, 73 insertions(+), 14 deletions(-)
+ sigemptyset(&sigmask); + sigaddset(&sigmask, SIGINT); + if (pthread_sigmask(SIG_BLOCK, &sigmask, &oldsigmask) < 0) + goto out_sig; + if (!vshConnectionUsability(ctl, ctl->conn)) - return false; + goto out;
Is it any more efficient to do the argument validation prior to the pthread_sigmask? I'd rather not since this code is executed in a separate thread which we want to ignore SIGINT. Otherwise we may hit situation where this thread receives the signal and therefore gets terminated without starting any job or writing anything to pipe.
+ + ret = do_watch_job(ctl, dom, verbose, p[0], 0, NULL, "Dump");
_("Dump")
ACK.

This patch alters saving code, so we can report progress and allow cancel via ^C. --- tools/virsh.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++--------- tools/virsh.pod | 8 ++-- 2 files changed, 76 insertions(+), 18 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index c2440e2..ab46b8c 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -2406,25 +2406,35 @@ static const vshCmdOptDef opts_save[] = { N_("filename containing updated XML for the target")}, {"running", VSH_OT_BOOL, 0, N_("set domain to be running on restore")}, {"paused", VSH_OT_BOOL, 0, N_("set domain to be paused on restore")}, + {"verbose", VSH_OT_BOOL, 0, N_("display the progress of save")}, {NULL, 0, 0, NULL} }; -static bool -cmdSave(vshControl *ctl, const vshCmd *cmd) +static void +doSave(void *opaque) { - virDomainPtr dom; + vshCtrlData *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; - bool ret = false; unsigned int flags = 0; const char *xmlfile = NULL; char *xml = NULL; + sigset_t sigmask, oldsigmask; + + sigemptyset(&sigmask); + sigaddset(&sigmask, SIGINT); + if (pthread_sigmask(SIG_BLOCK, &sigmask, &oldsigmask) < 0) + goto out_sig; if (!vshConnectionUsability(ctl, ctl->conn)) - return false; + goto out; if (vshCommandOptString(cmd, "file", &to) <= 0) - return false; + goto out; if (vshCommandOptBool(cmd, "bypass-cache")) flags |= VIR_DOMAIN_SAVE_BYPASS_CACHE; @@ -2435,29 +2445,77 @@ cmdSave(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptString(cmd, "xml", &xmlfile) < 0) { vshError(ctl, "%s", _("malformed xml argument")); - return false; + goto out; } if (!(dom = vshCommandOptDomain(ctl, cmd, &name))) - return false; + goto out; if (xmlfile && virFileReadAll(xmlfile, 8192, &xml) < 0) - goto cleanup; + goto out; if (((flags || xml) ? virDomainSaveFlags(dom, to, xml, flags) : virDomainSave(dom, to)) < 0) { vshError(ctl, _("Failed to save domain %s to %s"), name, to); - goto cleanup; + goto out; } - vshPrint(ctl, _("Domain %s saved to %s\n"), name, to); - ret = true; + ret = '0'; -cleanup: +out: + pthread_sigmask(SIG_SETMASK, &oldsigmask, NULL); +out_sig: + if (dom) virDomainFree (dom); VIR_FREE(xml); - virDomainFree(dom); + ignore_value(safewrite(data->writefd, &ret, sizeof(ret))); +} + +static bool +cmdSave(vshControl *ctl, const vshCmd *cmd) +{ + bool ret = false; + virDomainPtr dom = NULL; + int p[2] = {-1. -1}; + virThread workerThread; + bool verbose = false; + vshCtrlData data; + const char *to = NULL; + const char *name = NULL; + + if (!(dom = vshCommandOptDomain(ctl, cmd, &name))) + return false; + + if (vshCommandOptString(cmd, "file", &to) <= 0) + goto cleanup; + + if (vshCommandOptBool (cmd, "verbose")) + verbose = true; + + if (pipe(p) < 0) + goto cleanup; + + data.ctl = ctl; + data.cmd = cmd; + data.writefd = p[1]; + + if (virThreadCreate(&workerThread, + true, + doSave, + &data) < 0) + goto cleanup; + + ret = do_watch_job(ctl, dom, verbose, p[0], 0, NULL, "Save"); + + virThreadJoin(&workerThread); + + if (ret) + vshPrint(ctl, _("\nDomain %s saved to %s\n"), name, to); + +cleanup: + if (dom) + virDomainFree(dom); return ret; } diff --git a/tools/virsh.pod b/tools/virsh.pod index 5a5fbf8..0141728 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -875,7 +875,7 @@ have also reverted all storage volumes back to the same contents as when the state file was created. =item B<save> I<domain-id> I<state-file> [I<--bypass-cache>] [I<--xml> B<file>] -[{I<--running> | I<--paused>}] +[{I<--running> | I<--paused>}] [I<--verbose>] Saves a running domain (RAM, but not disk state) to a state file so that it can be restored @@ -886,9 +886,9 @@ If I<--bypass-cache> is specified, the save will avoid the file system cache, although this may slow down the operation. The progress may be monitored using B<domjobinfo> virsh command and canceled -with B<domjobabort> command (sent by another virsh instance). Interrupting -(usually with C<Ctrl-C>) the virsh process which runs B<save> command is not -enough to actually cancel the operation. +with B<domjobabort> command (sent by another virsh instance). Another option +is to send SIGINT (usually with C<Ctrl-C>) to the virsh process running +B<save> command. I<--verbose> displays the progress of save. This is roughly equivalent to doing a hibernate on a running computer, with all the same limitations. Open network connections may be -- 1.7.3.4

On 12/20/2011 08:21 AM, Michal Privoznik wrote:
This patch alters saving code, so we can report progress and allow cancel via ^C. --- tools/virsh.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++--------- tools/virsh.pod | 8 ++-- 2 files changed, 76 insertions(+), 18 deletions(-)
+ + ret = do_watch_job(ctl, dom, verbose, p[0], 0, NULL, "Save");
_("Save") ACK. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

This patch alters saving code, so we can report progress and allow cancel via ^C. --- tools/virsh.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++------- tools/virsh.pod | 8 +++--- 2 files changed, 68 insertions(+), 13 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index ab46b8c..6edac1e 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -2735,19 +2735,29 @@ static const vshCmdOptDef opts_managedsave[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, {"running", VSH_OT_BOOL, 0, N_("set domain to be running on next start")}, {"paused", VSH_OT_BOOL, 0, N_("set domain to be paused on next start")}, + {"verbose", VSH_OT_BOOL, 0, N_("display the progress of save")}, {NULL, 0, 0, NULL} }; -static bool -cmdManagedSave(vshControl *ctl, const vshCmd *cmd) +static void +doManagedsave(void *opaque) { - virDomainPtr dom; + char ret = '1'; + vshCtrlData *data = opaque; + vshControl *ctl = data->ctl; + const vshCmd *cmd = data->cmd; + virDomainPtr dom = NULL; const char *name; - bool ret = false; unsigned int flags = 0; + sigset_t sigmask, oldsigmask; + + sigemptyset(&sigmask); + sigaddset(&sigmask, SIGINT); + if (pthread_sigmask(SIG_BLOCK, &sigmask, &oldsigmask) < 0) + goto out_sig; if (!vshConnectionUsability(ctl, ctl->conn)) - return false; + goto out; if (vshCommandOptBool(cmd, "bypass-cache")) flags |= VIR_DOMAIN_SAVE_BYPASS_CACHE; @@ -2757,18 +2767,63 @@ cmdManagedSave(vshControl *ctl, const vshCmd *cmd) flags |= VIR_DOMAIN_SAVE_PAUSED; if (!(dom = vshCommandOptDomain(ctl, cmd, &name))) - return false; + goto out; if (virDomainManagedSave(dom, flags) < 0) { vshError(ctl, _("Failed to save domain %s state"), name); - goto cleanup; + goto out; } - vshPrint(ctl, _("Domain %s state saved by libvirt\n"), name); - ret = true; + ret = '0'; +out: + pthread_sigmask(SIG_SETMASK, &oldsigmask, NULL); +out_sig: + if (dom) + virDomainFree (dom); + ignore_value(safewrite(data->writefd, &ret, sizeof(ret))); +} + +static bool +cmdManagedSave(vshControl *ctl, const vshCmd *cmd) +{ + virDomainPtr dom; + int p[2] = { -1, -1}; + bool ret = false; + bool verbose = false; + const char *name = NULL; + vshCtrlData data; + virThread workerThread; + + if (!(dom = vshCommandOptDomain(ctl, cmd, &name))) + return false; + + if (vshCommandOptBool (cmd, "verbose")) + verbose = true; + + if (pipe(p) < 0) + goto cleanup; + + data.ctl = ctl; + data.cmd = cmd; + data.writefd = p[1]; + + if (virThreadCreate(&workerThread, + true, + doManagedsave, + &data) < 0) + goto cleanup; + + ret = do_watch_job(ctl, dom, verbose, p[0], 0, NULL, "Managedsave"); + + virThreadJoin(&workerThread); + + if (ret) + vshPrint(ctl, _("\nDomain %s state saved by libvirt\n"), name); cleanup: virDomainFree(dom); + VIR_FORCE_CLOSE(p[0]); + VIR_FORCE_CLOSE(p[1]); return ret; } diff --git a/tools/virsh.pod b/tools/virsh.pod index 0141728..bffc92f 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -733,7 +733,7 @@ The editor used can be supplied by the C<$VISUAL> or C<$EDITOR> environment variables, and defaults to C<vi>. =item B<managedsave> I<domain-id> [I<--bypass-cache>] -[{I<--running> | I<--paused>}] +[{I<--running> | I<--paused>}] [I<--verbose>] Save and destroy (stop) a running domain, so it can be restarted from the same state at a later time. When the virsh B<start> command is next run for @@ -742,9 +742,9 @@ If I<--bypass-cache> is specified, the save will avoid the file system cache, although this may slow down the operation. The progress may be monitored using B<domjobinfo> virsh command and canceled -with B<domjobabort> command (sent by another virsh instance). Interrupting -(usually with C<Ctrl-C>) the virsh process which runs B<managedsave> command -is not enough to actually cancel the operation. +with B<domjobabort> command (sent by another virsh instance). Another option +is to send SIGINT (usually with C<Ctrl-C>) to the virsh process running +B<managedsave> command. I<--verbose> displays the progress of save. Normally, starting a managed save will decide between running or paused based on the state the domain was in when the save was done; passing -- 1.7.3.4

On 12/20/2011 08:21 AM, Michal Privoznik wrote:
This patch alters saving code, so we can report progress and allow cancel via ^C. --- tools/virsh.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++------- tools/virsh.pod | 8 +++--- 2 files changed, 68 insertions(+), 13 deletions(-)
+ virDomainFree (dom); + ignore_value(safewrite(data->writefd, &ret, sizeof(ret))); +} + +static bool
Trailing space on that blank line. Make sure your series is 'syntax-check' clean.
+ + ret = do_watch_job(ctl, dom, verbose, p[0], 0, NULL, "Managedsave");
_("Managedsave") ACK. Depending on your reaction to my review on 1/4, you may have some minor rebasing to do on function names used in 2-4/4. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake
-
Michal Privoznik