Re: [Qemu-devel] [PATCH 3/6] migration: add support for a "tls-authz" migration parameter

[Juan Quintela <quintela@redhat.com>]

Daniel P. Berrangé <berrange@redhat.com> wrote:
> From: "Daniel P. Berrange" <berrange@redhat.com>

.....


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.