[libvirt] [PATCH v4 1/5] migration: add compress method option

[Jiri Denemark]

On Wed, Mar 23, 2016 at 13:22:45 +0300, Nikolay Shirokovskiy wrote:
> On 18.03.2016 16:47, Jiri Denemark wrote:
> >>  static int
> >> +qemuMigrationSetCompression(virQEMUDriverPtr driver,
> >> +                            virDomainObjPtr vm,
> >> +                            qemuDomainAsyncJob job,
> >> +                            qemuMigrationCompressionPtr compression,
> >> +                            unsigned int flags)
> >> +{
> >> +    /*
> >> +     * if compression methods are not set explicitly use flags to
> >> +     * set default compression methods
> >> +     */
> >> +    if (compression == NULL || compression->methods == 0) {
> >> +        return qemuMigrationSetOption(driver, vm,
> >> +                                      QEMU_MONITOR_MIGRATION_CAPS_XBZRLE,
> >> +                                      flags & VIR_MIGRATE_COMPRESSED,
> >> +                                      job);
> >> +    }
> > 
> > I think it would be cleaner if qemuMigrationCompressionParseParams got
> > the flags and set QEMU_MIGRATION_COMPESS_XBZRLE in compression->methods
> > in case no compression parametr is used. Doing that would even fix a bug
> > in your code, because even if no VIR_MIGRATE_PARAM_COMPRESSION parameter
> > is used, you still want to explicitly disable any supported compression
> > capability. In other words, you'd still have to call
> > qemuMigrationSetOption for both XBZRLE and MULTITHREAD in the if
> > statement above. Changing qemuMigrationCompressionParseParams to do the
> > right thing will avoid this code duplication.
> > 
> 
> Hi, Jiri. Thanx for reviewing.
> 
> I think we'd better stick with this approach.

It's quite unclear which approach you refer to by "this" :-)

> The reason is that we can get here thru old API where only flags are
> set and compression parameter is set to NULL on the way.

Yes, and if we always do the right thing in
qemuMigrationCompressionParseParams we don't need to think about
backward compatibility elsewhere and complicate the code there.

> >> +
> >> +    if (qemuMigrationSetOption(driver, vm,
> >> +                               QEMU_MONITOR_MIGRATION_CAPS_XBZRLE,
> >> +                               compression->methods & QEMU_MIGRATION_COMPESS_XBZRLE,
> >> +                               job) < 0)
> >> +        return -1;
> >> +
> >> +    if (qemuMigrationSetOption(driver, vm,
> >> +                               QEMU_MONITOR_MIGRATION_CAPS_COMPRESS,
> >> +                               compression->methods & QEMU_MIGRATION_COMPESS_MULTITHREAD,
> >> +                               job) < 0)
> >> +        return -1;
> >> +
> >> +    return 0;
> >> +}
> >> +
> >> +static int
> >>  qemuMigrationPrepareAny(virQEMUDriverPtr driver,
> >>                          virConnectPtr dconn,
> >>                          const char *cookiein,
> > ...
> >> @@ -6293,3 +6339,66 @@ qemuMigrationErrorReport(virQEMUDriverPtr driver,
> >>      virSetError(err);
> >>      virFreeError(err);
> >>  }
> >> +
> >> +static int
> >> +qemuMigrationCompressMethodsAdd(qemuMigrationCompressMethods *methods,
> >> +                                qemuMigrationCompressMethods method)
> >> +{
> >> +    if (*methods & method) {
> >> +        virReportError(VIR_ERR_INVALID_ARG, "%s",
> >> +                       _("Compression method is specified twice."));
> >> +        return -1;
> >> +    }
> >> +
> >> +    *methods |= method;
> >> +    return 0;
> >> +}
> > 
> > I don't think there is a reason to split the above trivial code into a
> > separate function. The main reason is that the following function should
> > be simplified.
> > 
> >> +
> >> +int
> >> +qemuMigrationCompressionParseParams(qemuMigrationCompressionPtr compression,
> >> +                                    virTypedParameterPtr params, int nparams)
> >> +{
> >> +    size_t i;
> >> +
> >> +    for (i = 0; i < nparams; i++) {
> >> +        if (STRNEQ(params[i].field, VIR_MIGRATE_PARAM_COMPRESSION))
> >> +            continue;
> > 
> > I think using virTypedParamsGetStringList to get all
> > VIR_MIGRATE_PARAM_COMPRESSION parameters and then walking through them
> > in a loop would look a bit nicer.
> > 
> 
> Well, this way we can remove the the check above at the price of introducing
> variables and memory managment. Also we are not going to use the extracted list outside of
> the function. Is it worth the effort?

The code would be a bit cleaner that way IMHO, but either way works.

> >> +
> >> +        if (STREQ(params[i].value.s, "xbzrle")) {
> >> +            if (qemuMigrationCompressMethodsAdd(&compression->methods,
> >> +                                                QEMU_MIGRATION_COMPESS_XBZRLE) < 0)
> >> +                return -1;
> >> +        } else if (STREQ(params[i].value.s, "multithread")) {
> >> +            if (qemuMigrationCompressMethodsAdd(&compression->methods,
> >> +                                                QEMU_MIGRATION_COMPESS_MULTITHREAD) < 0)
> >> +                return -1;
> >> +        } else {
> >> +            virReportError(VIR_ERR_INVALID_ARG, "%s",
> >> +                           _("Unsupported compression method"));
> >> +            return -1;
> >> +        }
> > 
> > Anyway, this is very ugly. You should change
> > qemuMigrationCompressMethods to be usable with our VIR_ENUM_{IMPL,DECL}
> > macros and use them here and in the *DumpParams function.
> 
> AFAIU I can not use VIR_ENUM_* family because I have flags and not sequential
> values in enum. I can introduce another family and set of related functions
> to deal with flags. Some generics to parse flags to virTypedParameterPtr
> back and forth. Do you think it is time to take this way?

Well, the enum should contain sequential numbers so that you can use
VIR_DENUM_* for it. The code enabling them can easily do 1 << method to
set the appropriate bit.

> 
> > 
> >> +    }
> > 
> > And set QEMU_MIGRATION_COMPESS_XBZRLE method if no param is passed and
> > flags enabled compression.
> > 
> >> +
> >> +    return 0;
> >> +}
> >> +
> >> +int
> >> +qemuMigrationCompressionDumpParams(qemuMigrationCompressionPtr compression,
> >> +                                   virTypedParameterPtr *params,
> >> +                                   int *nparams,
> >> +                                   int *maxparams)
> >> +{
> >> +    if ((compression->methods & QEMU_MIGRATION_COMPESS_XBZRLE) &&
> >> +        virTypedParamsAddString(params, nparams, maxparams,
> >> +                                VIR_MIGRATE_PARAM_COMPRESSION,
> >> +                                "xbzrle") < 0)
> >> +        return -1;
> >> +
> >> +    if ((compression->methods & QEMU_MIGRATION_COMPESS_MULTITHREAD) &&
> >> +        virTypedParamsAddString(params, nparams, maxparams,
> >> +                                VIR_MIGRATE_PARAM_COMPRESSION,
> >> +                                "multithread") < 0)
> >> +        return -1;
> > 
> > For backward compatibility this would just need to keep params empty if
> > xbzrle was the only compression method specified and use only flags to
> > express that.
> 
> I suggest that we don't mix flags and compression parameters here and
> in parse function too. This way we do not get backward compatibility
> issues. If VIR_MIGRATE_COMPRESSED is specified we don't touch
> it and do not convert to compression parameters. This way the old
> value space is passed transparently by the old means. On dump compression.methods
> will be 0 and we do not add params unrecognized by older libvirt.

The backward compatibility code will be limited to Parse/Dump parameters
and the rest of the code will just see a nice struct of compression
parameters without having to worry about how it was called. And dealing
with it in *Dump is easy, just set don't output any parameters if only
XBZRLE method was selected and no compression parameter is set.

Jirka