On 07/28/2017 10:57 AM, Scott Garfinkle wrote:
> From: Scott Garfinkle <seg@xxxxxxxxxx>
>
Need a commit message. Typically this is something like "Set up wire and
protocol for xxx" (see commit id 20ffaf59d for the SetMaxDowntime example)
> ---
> src/qemu/qemu_driver.c | 51 ++++++++++++++++++++++++++++++++++++++++++++
> src/qemu/qemu_monitor.h | 3 +++
> src/qemu/qemu_monitor_json.c | 4 ++++
> src/remote/remote_driver.c | 1 +
> src/remote/remote_protocol.x | 16 +++++++++++++-
> src/remote_protocol-structs | 8 +++++++
> 6 files changed, 82 insertions(+), 1 deletion(-)
>
This patch failed 'make check syntax-check' (see below)
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index f7b4cc3..8b8ae57 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -13147,6 +13147,56 @@ qemuDomainMigrateSetMaxDowntime(virDomainPtr dom,
> return ret;
> }
>
> +
> +static int
> +qemuDomainMigrateGetMaxDowntime(virDomainPtr dom,
> + unsigned long long *downtime,
> + unsigned int flags)
> +{
> + virQEMUDriverPtr driver = dom->conn->privateData;
> + virDomainObjPtr vm;
> + qemuDomainObjPrivatePtr priv;
> + qemuMonitorMigrationParams migparams;
s/migparams;/migparams = { 0 };
just to be clear - so that downtimeLimit_set isn't 1 due to random stack
stuff
> + int ret = -1;
> +
> + virCheckFlags(0, -1);
> +
> + if (!(vm = qemuDomObjFromDomain(dom)))
> + goto cleanup;
Could return -1 here
> +
> + if (virDomainMigrateGetMaxDowntimeEnsureACL(dom->conn, vm->def) < 0)
> + goto cleanup;
> +
> + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
> + goto cleanup;
> +
> + if (!virDomainObjIsActive(vm)) {
> + virReportError(VIR_ERR_OPERATION_INVALID,
> + "%s", _("domain is not running"));
> + goto endjob;
> + }
> +
> + priv = vm->privateData;
> + qemuDomainObjEnterMonitor(driver, vm);
> +
> + if (!(ret = qemuMonitorGetMigrationParams(priv->mon, &migparams))) {
I go back and forth on this "if (!(ret = ())) vs. "if ((ret = ()) == 0)"
- I like the latter mainly because when I see (!( I'm thinking pointers.
> + if (migparams.downtimeLimit_set)
> + *downtime = migparams.downtimeLimit;
> + else
> + ret = -1;
I think this needs some sort of error message; otherwise, if the
"downtime-limit" doesn't exist from a "query-migrate-parameters" then
you'd get the fallback libvirt failed for some unknown reason error message.
Other places that return -1 here would all elicit some message... Thus:
if (qemuMonitorGetMigrationParams(priv->mon, &migparams) == 0) {
if (migparams.downtimeLimit_set) {
*downtime = migparams.downtimeLimit;
ret = 0;
} else {
virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
_("Getting maximum downtime limit is not
supported"));
}
}
(or some error message such as that - I'm not sure if there is a way to
determine like the cache code does that the parameter exists.
> + }
> +
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + ret = -1;
> +
> + endjob:
> + qemuDomainObjEndJob(driver, vm);
> +
> + cleanup:
> + virDomainObjEndAPI(&vm);
> + return ret;
> +}
> +
Extra space after too...
> static int
> qemuDomainMigrateGetCompressionCache(virDomainPtr dom,
> unsigned long long *cacheSize,
> @@ -20826,6 +20876,7 @@ static virHypervisorDriver qemuHypervisorDriver = {
> .domainGetJobInfo = qemuDomainGetJobInfo, /* 0.7.7 */
> .domainGetJobStats = qemuDomainGetJobStats, /* 1.0.3 */
> .domainAbortJob = qemuDomainAbortJob, /* 0.7.7 */
> + .domainMigrateGetMaxDowntime = qemuDomainMigrateGetMaxDowntime, /* 3.6.0 */
3.7.0
> .domainMigrateSetMaxDowntime = qemuDomainMigrateSetMaxDowntime, /* 0.8.0 */
> .domainMigrateGetCompressionCache = qemuDomainMigrateGetCompressionCache, /* 1.0.3 */
> .domainMigrateSetCompressionCache = qemuDomainMigrateSetCompressionCache, /* 1.0.3 */
> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> index 31f7e97..9805a33 100644
> --- a/src/qemu/qemu_monitor.h
> +++ b/src/qemu/qemu_monitor.h
> @@ -627,6 +627,9 @@ struct _qemuMonitorMigrationParams {
> * whereas, some string value indicates we can support setting/clearing */
> char *migrateTLSAlias;
> char *migrateTLSHostname;
> +
> + bool downtimeLimit_set;
> + unsigned long long downtimeLimit;
> };
>
> int qemuMonitorGetMigrationParams(qemuMonitorPtr mon,
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index b8a6815..b7b809d 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -2703,6 +2703,10 @@ qemuMonitorJSONGetMigrationParams(qemuMonitorPtr mon,
> PARSE(cpuThrottleInitial, "cpu-throttle-initial");
> PARSE(cpuThrottleIncrement, "cpu-throttle-increment");
>
> + if (virJSONValueObjectGetNumberUlong(result, "downtime-limit",
> + ¶ms->downtimeLimit) == 0)
> + params->downtimeLimit_set = true;
> +
> #undef PARSE
Put the new code after the #undef since it's not part of the PARSE
>
> if ((tlsStr = virJSONValueObjectGetString(result, "tls-creds"))) {
> diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
> index a57d25f..aa8d8a1 100644
> --- a/src/remote/remote_driver.c
> +++ b/src/remote/remote_driver.c
> @@ -8400,6 +8400,7 @@ static virHypervisorDriver hypervisor_driver = {
> .domainGetJobInfo = remoteDomainGetJobInfo, /* 0.7.7 */
> .domainGetJobStats = remoteDomainGetJobStats, /* 1.0.3 */
> .domainAbortJob = remoteDomainAbortJob, /* 0.7.7 */
> + .domainMigrateGetMaxDowntime = remoteDomainMigrateGetMaxDowntime, /* 3.6.0 */
3.7.0
> .domainMigrateSetMaxDowntime = remoteDomainMigrateSetMaxDowntime, /* 0.8.0 */
> .domainMigrateGetCompressionCache = remoteDomainMigrateGetCompressionCache, /* 1.0.3 */
> .domainMigrateSetCompressionCache = remoteDomainMigrateSetCompressionCache, /* 1.0.3 */
> diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x
> index aa0aa38..e1f4e27 100644
> --- a/src/remote/remote_protocol.x
> +++ b/src/remote/remote_protocol.x
> @@ -2326,6 +2326,15 @@ struct remote_domain_abort_job_args {
> };
>
>
> +struct remote_domain_migrate_get_max_downtime_args {
> + remote_nonnull_domain dom;
> + unsigned int flags;
> +};
> +
> +struct remote_domain_migrate_get_max_downtime_ret {
> + unsigned hyper downtime; /* insert@1 */
> +};
> +
> struct remote_domain_migrate_set_max_downtime_args {
> remote_nonnull_domain dom;
> unsigned hyper downtime;
> @@ -6064,7 +6073,12 @@ enum remote_procedure {
> * @generate: both
> * @acl: domain:write
> */
> - REMOTE_PROC_DOMAIN_SET_BLOCK_THRESHOLD = 386
> + REMOTE_PROC_DOMAIN_SET_BLOCK_THRESHOLD = 386,
>
> + /**
> + * @generate: both
> + * @acl: domain:migrate
> + */
> + REMOTE_PROC_DOMAIN_MIGRATE_GET_MAX_DOWNTIME = 387
>
> };
> diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs
> index a46fe37..5804e60 100644
> --- a/src/remote_protocol-structs
> +++ b/src/remote_protocol-structs
> @@ -1778,6 +1778,13 @@ struct remote_domain_migrate_set_max_downtime_args {
> uint64_t downtime;
> u_int flags;
> };
> +struct remote_domain_migrate_get_max_downtime_args {
> + remote_nonnull_domain dom;
> + u_int flags;
> +};
> +struct remote_domain_migrate_get_max_downtime_ret {
> + uint64_t downtime;
> +};
These two should be above the "set_max_downtime_args"
(check/syntax-check failure)
> struct remote_domain_migrate_get_compression_cache_args {
> remote_nonnull_domain dom;
> u_int flags;
> @@ -3233,4 +3240,5 @@ enum remote_procedure {
> REMOTE_PROC_DOMAIN_SET_VCPU = 384,
> REMOTE_PROC_DOMAIN_EVENT_BLOCK_THRESHOLD = 385,
> REMOTE_PROC_DOMAIN_SET_BLOCK_THRESHOLD = 386,
> + REMOTE_PROC_DOMAIN_GET_MAX_DOWNTIME = 387
This needs to be:
REMOTE_PROC_DOMAIN_MIGRATE_GET_MAX_DOWNTIME = 387,
from check/syntax-check failure
John
> };
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
