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

[Eric Blake]

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 at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20111221/84b53cd8/attachment-0001.sig>