[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH 3/6] migration: add support for a "tls-authz" mi
From: |
Juan Quintela |
Subject: |
Re: [Qemu-block] [PATCH 3/6] migration: add support for a "tls-authz" migration parameter |
Date: |
Wed, 20 Jun 2018 12:03:45 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) |
Daniel P. Berrangé <address@hidden> wrote:
> From: "Daniel P. Berrange" <address@hidden>
.....
It is not just the fault of this patch, but as you are the one doing the
tls bits on migration...
> @@ -1106,6 +1108,12 @@ static void migrate_params_apply(MigrateSetParameters
> *params, Error **errp)
> s->parameters.tls_hostname = g_strdup(params->tls_hostname->u.s);
> }
>
> + if (params->has_tls_authz) {
> + g_free(s->parameters.tls_authz);
> + assert(params->tls_authz->type == QTYPE_QSTRING);
We really try hard not to use assert() on migration code (yes, it is an
ongoing effort). The code around this is something like:
static void migrate_params_test_apply(MigrateSetParameters *params,
MigrationParameters *dest)
{
[...]
if (params->has_compress_level) {
dest->compress_level = params->compress_level;
}
[...]
if (params->has_tls_creds) {
assert(params->tls_creds->type == QTYPE_QSTRING);
dest->tls_creds = g_strdup(params->tls_creds->u.s);
}
[...]
}
Ok, tls code is the one with strings, but still.
static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
{
[...]
if (params->has_compress_level) {
s->parameters.compress_level = params->compress_level;
}
[...]
if (params->has_tls_creds) {
g_free(s->parameters.tls_creds);
assert(params->tls_creds->type == QTYPE_QSTRING);
s->parameters.tls_creds = g_strdup(params->tls_creds->u.s);
}
}
And this other function:
static bool migrate_params_check(MigrationParameters *params, Error **errp)
{
if (params->has_compress_level &&
(params->compress_level > 9)) {
error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "compress_level",
"is invalid, it should be in the range of 0 to 9");
return false;
}
[...]
}
Where we don't check anything for tls.
Perhaps we can move the asserts here?
We can also try to merge migrate_params_check and
migrate_params_test_apply() into a single function, but it is not
completely trivial at the moment.
Wondering if we can do it better.
Later, Juan.