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