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

[Michal Privoznik]

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.