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@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list