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.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list