On Mon, Jan 29, 2018 at 11:32:04 -0500, John Ferlan wrote:
> Convert the stats field in _qemuDomainJobInfo to be a union. This
> will allow for the collection of various different types of stats
> in the same field. While doing this, also change the name of the
> field from @stats to @migStats to make it easier to find.
>
> When starting the async job that will end up being used for stats,
> set the @statsType value appropriately. The @mirrorStats are
> special and are used with @migStats in order to generate the
> returned job stats for a migration.
>
> Using the NONE should avoid the possibility that some random
> async job would try to return stats for migration even though
> a migration is not in progress.
>
> For now a migration and a save job will use the same statsType
>
> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
> ---
> src/qemu/qemu_domain.c | 63 ++++++++++++++++++++++++++++------------
> src/qemu/qemu_domain.h | 12 +++++++-
> src/qemu/qemu_driver.c | 19 +++++++++---
> src/qemu/qemu_migration.c | 13 +++++----
> src/qemu/qemu_migration_cookie.c | 4 +--
> src/qemu/qemu_process.c | 2 +-
> 6 files changed, 82 insertions(+), 31 deletions(-)
If you make this change first, you can just add dump stats into the
union afterwards and use it in the rest of the patches instead of having
two dump stats structs.
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index d8b2b3067..bf9e12271 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -413,8 +413,8 @@ qemuDomainJobInfoUpdateDowntime(qemuDomainJobInfoPtr jobInfo)
> return 0;
> }
>
> - jobInfo->stats.downtime = now - jobInfo->stopped;
> - jobInfo->stats.downtime_set = true;
> + jobInfo->s.migStats.downtime = now - jobInfo->stopped;
> + jobInfo->s.migStats.downtime_set = true;
I think maybe jobInfo->stats.mig... and jobInfo->stats.dump... would be
a bit better. And it would even be shorter :-)
> return 0;
> }
>
> @@ -452,17 +452,24 @@ qemuDomainJobInfoToInfo(qemuDomainJobInfoPtr jobInfo,
> info->type = qemuDomainJobStatusToType(jobInfo->status);
> info->timeElapsed = jobInfo->timeElapsed;
>
> - info->memTotal = jobInfo->stats.ram_total;
> - info->memRemaining = jobInfo->stats.ram_remaining;
> - info->memProcessed = jobInfo->stats.ram_transferred;
> + switch ((qemuDomainJobStatsType) jobInfo->statsType) {
No need to typecast it here as statsType is already declared as enum
rather than int.
> + case QEMU_DOMAIN_JOB_STATS_TYPE_MIGRATION:
> + info->memTotal = jobInfo->s.migStats.ram_total;
> + info->memRemaining = jobInfo->s.migStats.ram_remaining;
> + info->memProcessed = jobInfo->s.migStats.ram_transferred;
> + info->fileTotal = jobInfo->s.migStats.disk_total +
> + jobInfo->mirrorStats.total;
> + info->fileRemaining = jobInfo->s.migStats.disk_remaining +
> + (jobInfo->mirrorStats.total -
> + jobInfo->mirrorStats.transferred);
> + info->fileProcessed = jobInfo->s.migStats.disk_transferred +
> + jobInfo->mirrorStats.transferred;
> + break;
>
> - info->fileTotal = jobInfo->stats.disk_total +
> - jobInfo->mirrorStats.total;
> - info->fileRemaining = jobInfo->stats.disk_remaining +
> - (jobInfo->mirrorStats.total -
> - jobInfo->mirrorStats.transferred);
> - info->fileProcessed = jobInfo->stats.disk_transferred +
> - jobInfo->mirrorStats.transferred;
> + case QEMU_DOMAIN_JOB_STATS_TYPE_NONE:
> + case QEMU_DOMAIN_JOB_STATS_TYPE_LAST:
> + break;
> + }
>
> info->dataTotal = info->memTotal + info->fileTotal;
> info->dataRemaining = info->memRemaining + info->fileRemaining;
> @@ -471,13 +478,14 @@ qemuDomainJobInfoToInfo(qemuDomainJobInfoPtr jobInfo,
> return 0;
> }
>
> -int
> -qemuDomainJobInfoToParams(qemuDomainJobInfoPtr jobInfo,
> - int *type,
> - virTypedParameterPtr *params,
> - int *nparams)
> +
> +static int
> +qemuDomainMigrationJobInfoToParams(qemuDomainJobInfoPtr jobInfo,
> + int *type,
> + virTypedParameterPtr *params,
> + int *nparams)
> {
> - qemuMonitorMigrationStats *stats = &jobInfo->stats;
> + qemuMonitorMigrationStats *stats = &jobInfo->s.migStats;
> qemuDomainMirrorStatsPtr mirrorStats = &jobInfo->mirrorStats;
> virTypedParameterPtr par = NULL;
> int maxpar = 0;
> @@ -639,6 +647,25 @@ qemuDomainJobInfoToParams(qemuDomainJobInfoPtr jobInfo,
> }
>
>
> +int
> +qemuDomainJobInfoToParams(qemuDomainJobInfoPtr jobInfo,
> + int *type,
> + virTypedParameterPtr *params,
> + int *nparams)
> +{
> + switch ((qemuDomainJobStatsType) jobInfo->statsType) {
No typecast is needed here either.
> + case QEMU_DOMAIN_JOB_STATS_TYPE_MIGRATION:
> + return qemuDomainMigrationJobInfoToParams(jobInfo, type, params, nparams);
> +
> + case QEMU_DOMAIN_JOB_STATS_TYPE_NONE:
> + case QEMU_DOMAIN_JOB_STATS_TYPE_LAST:
> + break;
> + }
> +
> + return -1;
> +}
> +
> +
> /* qemuDomainGetMasterKeyFilePath:
> * @libDir: Directory path to domain lib files
> *
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index 7dab758fb..d4cad72b9 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -110,6 +110,13 @@ typedef enum {
> QEMU_DOMAIN_JOB_STATUS_CANCELED,
> } qemuDomainJobStatus;
>
> +typedef enum {
> + QEMU_DOMAIN_JOB_STATS_TYPE_NONE = 0,
> + QEMU_DOMAIN_JOB_STATS_TYPE_MIGRATION,
> +
> + QEMU_DOMAIN_JOB_STATS_TYPE_LAST
_LAST is only required when we want to add VIR_ENUM_{DECL,IMPL} since
the string to enum converter needs to know the number of items of the
enum. However, we don't need or want this conversion here. Just drop
QEMU_DOMAIN_JOB_STATS_TYPE_LAST.
> +} qemuDomainJobStatsType;
> +
>
> typedef struct _qemuDomainMirrorStats qemuDomainMirrorStats;
> typedef qemuDomainMirrorStats *qemuDomainMirrorStatsPtr;
> @@ -138,7 +145,10 @@ struct _qemuDomainJobInfo {
> destination. */
> bool timeDeltaSet;
> /* Raw values from QEMU */
> - qemuMonitorMigrationStats stats;
> + qemuDomainJobStatsType statsType;
> + union {
> + qemuMonitorMigrationStats migStats;
> + } s;
> qemuDomainMirrorStats mirrorStats;
> };
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 00a010b45..b9c720221 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -3386,6 +3386,8 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver, virDomainPtr dom,
> goto endjob;
> }
>
> + priv->job.current->statsType = QEMU_DOMAIN_JOB_STATS_TYPE_MIGRATION;
> +
> /* Pause */
> if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) {
> was_running = true;
> @@ -3937,6 +3939,9 @@ qemuDomainCoreDumpWithFormat(virDomainPtr dom,
> goto endjob;
> }
>
> + priv = vm->privateData;
> + priv->job.current->statsType = QEMU_DOMAIN_JOB_STATS_TYPE_MIGRATION;
> +
> /* Migrate will always stop the VM, so the resume condition is
> independent of whether the stop command is issued. */
> resume = virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING;
> @@ -3972,7 +3977,6 @@ qemuDomainCoreDumpWithFormat(virDomainPtr dom,
> } else if (((resume && paused) || (flags & VIR_DUMP_RESET)) &&
> virDomainObjIsActive(vm)) {
> if ((ret == 0) && (flags & VIR_DUMP_RESET)) {
> - priv = vm->privateData;
> qemuDomainObjEnterMonitor(driver, vm);
> ret = qemuMonitorSystemReset(priv->mon);
> if (qemuDomainObjExitMonitor(driver, vm) < 0)
> @@ -13226,10 +13230,17 @@ qemuDomainGetJobStatsInternal(virQEMUDriverPtr driver,
> }
> *jobInfo = *priv->job.current;
>
> - if (qemuDomainGetJobInfoMigrationStats(driver, vm, jobInfo) < 0)
> - goto cleanup;
> + switch ((qemuDomainJobStatsType) jobInfo->statsType) {
Redundant typecast.
> + case QEMU_DOMAIN_JOB_STATS_TYPE_MIGRATION:
> + if (qemuDomainGetJobInfoMigrationStats(driver, vm, jobInfo) < 0)
> + goto cleanup;
> + ret = 0;
> + break;
>
> - ret = 0;
> + case QEMU_DOMAIN_JOB_STATS_TYPE_NONE:
> + case QEMU_DOMAIN_JOB_STATS_TYPE_LAST:
> + break;
> + }
>
> cleanup:
> qemuDomainObjEndJob(driver, vm);
...
Jirka
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
