Luiz Capitulino <lcapitul...@redhat.com> wrote: > On Wed, 11 Jul 2012 12:25:38 -0600 > Eric Blake <ebl...@redhat.com> wrote: > >> On 07/11/2012 12:08 PM, Luiz Capitulino wrote: >> > On Fri, 29 Jun 2012 18:43:57 +0200 >> > Juan Quintela <quint...@redhat.com> wrote: >> > >> >> We add time spent for migration to the output of "info migrate" >> >> command. 'total_time' means time since the start fo migration if >> >> migration is 'active', and total time of migration if migration is >> >> completed. As we are also interested in transferred ram when >> >> migration completes, adding all ram statistics >> > >> > I see this has already been merged and am sorry for being late with my >> > review, but it turns out that there are a few issues to be addressed in >> > this patch, comments inlined below. >> > >> > Another point is that this patch extends the query-migrate command. We've >> > decided not to extend QMP commands, however I think that we should relax >> > that restriction for query commands, because the client doesn't need to >> > know >> > the new fields in advance. >> >> I see a difference between extending output (such as query command >> return values), where clients can gracefully deal with the absence of a >> parameter from an older qemu, and extending input (where it is hard to >> tell up-front whether a parameter will be rejected by an older qemu). >> This is a case of extending output, so I am okay with it; in fact, I'd >> rather extend output of existing commands than add a new command, >> because then libvirt has to probe which command to call, instead of >> calling one command and then parsing everything available. > > Yes, agreed. At least until we have libqmp... > >> >> + monitor_printf(mon, "total time: %" PRIu64 " milliseconds\n", >> >> + info->ram->total_time); >> > >> > This adds a new line to the HMP output between the end of the ram stats and >> > the disk stats. Iirc libvirt parses this output when in non-json >> > mode, although >> > I don't think it ever does it for disk migration. >> > >> > Eric, does libvirt do that? >> >> Libvirt forces the use of QMP for qemu 0.15 and newer. So while there >> may be other users that care, libvirt could care less whether HMP output >> changes between qemu 1.1 and 1.2. > > Good to know. > >> >> >> +++ b/migration.c >> >> @@ -131,6 +131,8 @@ MigrationInfo *qmp_query_migrate(Error **errp) >> >> info->ram->transferred = ram_bytes_transferred(); >> >> info->ram->remaining = ram_bytes_remaining(); >> >> info->ram->total = ram_bytes_total(); >> >> + info->ram->total_time = qemu_get_clock_ms(rt_clock) >> >> + - s->total_time; >> >> >> > >> > I really don't think that 'total_time' pertains to the ram stats >> > info, I think >> > it should be in the MigrationInfo dict. >> >> If so, now's the time to change it before it becomes locked in stone >> with the qemu 1.2 release. Libvirt has not yet been taught to use the >> new information. > > Juan, if you agree with the change I can do it myself, or you could submit the > patch and I'll apply it to the QMP tree.
I can send a patch. Thanks for the review, Juan.
