Re: [libvirt] [PATCH 1/4] virsh: Move job watch code to a separate function

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.