| Message ID | 1433834286-13824-1-git-send-email-zhang.zhanghailiang@huawei.com |
|---|---|
| State | New |
| Headers | show |
On 06/09/2015 01:18 AM, zhanghailiang wrote: In the subject: s/easily/easier/ > 'migrate-set-parameters' was specially used for setting the compression related > parameters of migration. Here we re-implement it so that it can be easily extended when > we want to add other parameters for migration. Might want to make it clear that this is an ABI change, but on an unreleased command; it is safe to take this patch only if it makes it into 2.4. You claim that this makes it easier for extension, but don't go into details of why that is so; I guess it is that segregating related parameters into sub-structs makes it possible to pass sub-structs around instead of a list of parameters. > > Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> > --- > hmp.c | 23 +++++++++++-------- > migration/migration.c | 63 +++++++++++++++++++++++++++------------------------ > qapi-schema.json | 20 ++++++++++++---- > qmp-commands.hx | 4 ++-- > 4 files changed, 65 insertions(+), 45 deletions(-) > > diff --git a/hmp.c b/hmp.c > index e17852d..4305ae1 100644 > --- a/hmp.c > +++ b/hmp.c > @@ -1213,27 +1213,32 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict) > const char *param = qdict_get_str(qdict, "parameter"); > int value = qdict_get_int(qdict, "value"); > Error *err = NULL; > - bool has_compress_level = false; > - bool has_compress_threads = false; > - bool has_decompress_threads = false; > + struct MigrationCompressParameter *compress_para = > + g_malloc0(sizeof(*compress_para)); Do you really need to malloc this? Just stack-allocate it. Particularly since... > + bool has_compress_parameter = false; > int i; > > for (i = 0; i < MIGRATION_PARAMETER_MAX; i++) { > if (strcmp(param, MigrationParameter_lookup[i]) == 0) { > switch (i) { > case MIGRATION_PARAMETER_COMPRESS_LEVEL: > - has_compress_level = true; > + compress_para->has_compress_level = true; > + compress_para->compress_level = value; > + has_compress_parameter = true; > break; > case MIGRATION_PARAMETER_COMPRESS_THREADS: > - has_compress_threads = true; > + compress_para->has_compress_threads = true; > + compress_para->compress_threads = value; > + has_compress_parameter = true; > break; > case MIGRATION_PARAMETER_DECOMPRESS_THREADS: > - has_decompress_threads = true; > + compress_para->has_decompress_threads = true; > + compress_para->decompress_threads = value; > + has_compress_parameter = true; > break; > } > - qmp_migrate_set_parameters(has_compress_level, value, > - has_compress_threads, value, > - has_decompress_threads, value, > + qmp_migrate_set_parameters(has_compress_parameter, > + compress_para, > &err); > break; > } ...you forgot to free it, and therefore have a leak. I also think you can get away without using has_compress_parameter, and just call qmp_migrate_set_parameters(true, &compress_para, &err). Since all the members are optional, that should still do the right thing (in QMP terms, omitting the argument and passing 'compress-parameter':{} will do the same thing). > diff --git a/migration/migration.c b/migration/migration.c > index 732d229..eef0e5c 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -316,44 +316,49 @@ void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params, > } > } > > -void qmp_migrate_set_parameters(bool has_compress_level, > - int64_t compress_level, > - bool has_compress_threads, > - int64_t compress_threads, > - bool has_decompress_threads, > - int64_t decompress_threads, Error **errp) > +void qmp_migrate_set_parameters(bool has_compress_parameter, > + struct MigrationCompressParameter *compress_para, > + Error **errp) Sounds verbose, and makes you run up against 80-column limits. I wonder if any shorter names will work. > { > MigrationState *s = migrate_get_current(); > > - if (has_compress_level && (compress_level < 0 || compress_level > 9)) { > - error_set(errp, QERR_INVALID_PARAMETER_VALUE, "compress_level", > + if (has_compress_parameter) { > + if (compress_para->has_compress_level && > + (compress_para->compress_level < 0 || > + compress_para->compress_level > 9)) { > + error_set(errp, QERR_INVALID_PARAMETER_VALUE, "compress_level", > "is invalid, it should be in the range of 0 to 9"); A lot of verbosity. This could read as: if (compression->has_level && (compression->level < 0 || compression->level > 9)) { if you do some renaming... > - return; > - } > - if (has_compress_threads && > - (compress_threads < 1 || compress_threads > 255)) { > - error_set(errp, QERR_INVALID_PARAMETER_VALUE, > + return; > + } > + if (compress_para->has_compress_threads && > + (compress_para->compress_threads < 1 || > + compress_para->compress_threads > 255)) { ...except then we have a problem with compress_threads vs. decompress_threads. > +++ b/qapi-schema.json > @@ -592,10 +592,8 @@ > { 'enum': 'MigrationParameter', > 'data': ['compress-level', 'compress-threads', 'decompress-threads'] } > > -# > -# @migrate-set-parameters > -# > -# Set the following migration parameters > +## > +# @MigrationCompressParameter > # > # @compress-level: compression level Missing an overview of the purpose of this struct, which is to collect compression-related parameters into one spot. Back to the renaming bit, it feels redundant to name all parameters beginning 'compress-*' if the struct itself has Compress in the name (but that pesky decompress-threads bucks the trend). On the other hand, if you call this 'MigrationCompression'... > # > @@ -605,12 +603,24 @@ > # > # Since: 2.4 > ## > -{ 'command': 'migrate-set-parameters', > +{ 'struct': 'MigrationCompressParameter', > 'data': { '*compress-level': 'int', > '*compress-threads': 'int', > '*decompress-threads': 'int'} } > > # > +# @migrate-set-parameters > +# > +# Set the migration parameters > +# > +# @compress-parameter: compression related parameters > +# > +# Since: 2.4 > +## > +{ 'command': 'migrate-set-parameters', > + 'data': { '*compress-parameter': 'MigrationCompressParameter'} } ...and name the member '*compression':'MigrationCompression', that will still be legible, and buy you back some screen width. > + > +# > # @MigrationParameters > # > # @compress-level: compression level > diff --git a/qmp-commands.hx b/qmp-commands.hx > index 14e109e..ce721ba 100644 > --- a/qmp-commands.hx > +++ b/qmp-commands.hx > @@ -3460,14 +3460,14 @@ Arguments: > Example: > > -> { "execute": "migrate-set-parameters" , "arguments": > - { "compress-level": 1 } } > + { "compress-parameter": { "compress-level": 1 } } } and back to my argument of verbosity, doesn't: { "compression": { "level": 1 } } } look better? > > EQMP > > { > .name = "migrate-set-parameters", > .args_type = > - "compress-level:i?,compress-threads:i?,decompress-threads:i?", > + "compress-parameter:q?", > .mhandler.cmd_new = qmp_marshal_input_migrate_set_parameters, > }, > SQMP > Overall, I think the idea has merit, but it will need a v2.
On 2015/6/9 18:49, Eric Blake wrote: > On 06/09/2015 01:18 AM, zhanghailiang wrote: > > In the subject: s/easily/easier/ > >> 'migrate-set-parameters' was specially used for setting the compression related >> parameters of migration. Here we re-implement it so that it can be easily extended when >> we want to add other parameters for migration. > > Might want to make it clear that this is an ABI change, but on an > unreleased command; it is safe to take this patch only if it makes it > into 2.4. > > You claim that this makes it easier for extension, but don't go into > details of why that is so; I guess it is that segregating related > parameters into sub-structs makes it possible to pass sub-structs around > instead of a list of parameters. > Yes, this is the reason, i want to use this new command to set parameter for COLO, and Dave reminded me that there already has a discussion on it: https://lists.nongnu.org/archive/html/qemu-devel/2015-06/msg01709.html and i think maybe he wants to use this command for postcopy too? >> >> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> >> --- >> hmp.c | 23 +++++++++++-------- >> migration/migration.c | 63 +++++++++++++++++++++++++++------------------------ >> qapi-schema.json | 20 ++++++++++++---- >> qmp-commands.hx | 4 ++-- >> 4 files changed, 65 insertions(+), 45 deletions(-) >> >> diff --git a/hmp.c b/hmp.c >> index e17852d..4305ae1 100644 >> --- a/hmp.c >> +++ b/hmp.c >> @@ -1213,27 +1213,32 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict) >> const char *param = qdict_get_str(qdict, "parameter"); >> int value = qdict_get_int(qdict, "value"); >> Error *err = NULL; >> - bool has_compress_level = false; >> - bool has_compress_threads = false; >> - bool has_decompress_threads = false; >> + struct MigrationCompressParameter *compress_para = >> + g_malloc0(sizeof(*compress_para)); > > Do you really need to malloc this? Just stack-allocate it. Particularly > since... > Hmm, yes, stack-allocate seems more better. >> + bool has_compress_parameter = false; >> int i; >> >> for (i = 0; i < MIGRATION_PARAMETER_MAX; i++) { >> if (strcmp(param, MigrationParameter_lookup[i]) == 0) { >> switch (i) { >> case MIGRATION_PARAMETER_COMPRESS_LEVEL: >> - has_compress_level = true; >> + compress_para->has_compress_level = true; >> + compress_para->compress_level = value; >> + has_compress_parameter = true; >> break; >> case MIGRATION_PARAMETER_COMPRESS_THREADS: >> - has_compress_threads = true; >> + compress_para->has_compress_threads = true; >> + compress_para->compress_threads = value; >> + has_compress_parameter = true; >> break; >> case MIGRATION_PARAMETER_DECOMPRESS_THREADS: >> - has_decompress_threads = true; >> + compress_para->has_decompress_threads = true; >> + compress_para->decompress_threads = value; >> + has_compress_parameter = true; >> break; >> } >> - qmp_migrate_set_parameters(has_compress_level, value, >> - has_compress_threads, value, >> - has_decompress_threads, value, >> + qmp_migrate_set_parameters(has_compress_parameter, >> + compress_para, >> &err); >> break; >> } > > ...you forgot to free it, and therefore have a leak. > oops~ > I also think you can get away without using has_compress_parameter, and > just call qmp_migrate_set_parameters(true, &compress_para, &err). Since > all the members are optional, that should still do the right thing (in > QMP terms, omitting the argument and passing 'compress-parameter':{} > will do the same thing). > Good catch. >> diff --git a/migration/migration.c b/migration/migration.c >> index 732d229..eef0e5c 100644 >> --- a/migration/migration.c >> +++ b/migration/migration.c >> @@ -316,44 +316,49 @@ void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params, >> } >> } >> >> -void qmp_migrate_set_parameters(bool has_compress_level, >> - int64_t compress_level, >> - bool has_compress_threads, >> - int64_t compress_threads, >> - bool has_decompress_threads, >> - int64_t decompress_threads, Error **errp) >> +void qmp_migrate_set_parameters(bool has_compress_parameter, >> + struct MigrationCompressParameter *compress_para, >> + Error **errp) > > Sounds verbose, and makes you run up against 80-column limits. I wonder > if any shorter names will work. > Yes, maybe should change it to 'struct CompressParameter'. >> { >> MigrationState *s = migrate_get_current(); >> >> - if (has_compress_level && (compress_level < 0 || compress_level > 9)) { >> - error_set(errp, QERR_INVALID_PARAMETER_VALUE, "compress_level", >> + if (has_compress_parameter) { >> + if (compress_para->has_compress_level && >> + (compress_para->compress_level < 0 || >> + compress_para->compress_level > 9)) { >> + error_set(errp, QERR_INVALID_PARAMETER_VALUE, "compress_level", >> "is invalid, it should be in the range of 0 to 9"); > > A lot of verbosity. This could read as: > > if (compression->has_level && > (compression->level < 0 || compression->level > 9)) { > > if you do some renaming... > > >> - return; >> - } >> - if (has_compress_threads && >> - (compress_threads < 1 || compress_threads > 255)) { >> - error_set(errp, QERR_INVALID_PARAMETER_VALUE, >> + return; >> + } >> + if (compress_para->has_compress_threads && >> + (compress_para->compress_threads < 1 || >> + compress_para->compress_threads > 255)) { > > ...except then we have a problem with compress_threads vs. > decompress_threads. > >> +++ b/qapi-schema.json >> @@ -592,10 +592,8 @@ >> { 'enum': 'MigrationParameter', >> 'data': ['compress-level', 'compress-threads', 'decompress-threads'] } >> >> -# >> -# @migrate-set-parameters >> -# >> -# Set the following migration parameters >> +## >> +# @MigrationCompressParameter >> # >> # @compress-level: compression level > > Missing an overview of the purpose of this struct, which is to collect > compression-related parameters into one spot. > > Back to the renaming bit, it feels redundant to name all parameters > beginning 'compress-*' if the struct itself has Compress in the name > (but that pesky decompress-threads bucks the trend). On the other hand, > if you call this 'MigrationCompression'... > >> # >> @@ -605,12 +603,24 @@ >> # >> # Since: 2.4 >> ## >> -{ 'command': 'migrate-set-parameters', >> +{ 'struct': 'MigrationCompressParameter', >> 'data': { '*compress-level': 'int', >> '*compress-threads': 'int', >> '*decompress-threads': 'int'} } >> >> # >> +# @migrate-set-parameters >> +# >> +# Set the migration parameters >> +# >> +# @compress-parameter: compression related parameters >> +# >> +# Since: 2.4 >> +## >> +{ 'command': 'migrate-set-parameters', >> + 'data': { '*compress-parameter': 'MigrationCompressParameter'} } > > ...and name the member '*compression':'MigrationCompression', that will > still be legible, and buy you back some screen width. > >> + >> +# >> # @MigrationParameters >> # >> # @compress-level: compression level >> diff --git a/qmp-commands.hx b/qmp-commands.hx >> index 14e109e..ce721ba 100644 >> --- a/qmp-commands.hx >> +++ b/qmp-commands.hx >> @@ -3460,14 +3460,14 @@ Arguments: >> Example: >> >> -> { "execute": "migrate-set-parameters" , "arguments": >> - { "compress-level": 1 } } >> + { "compress-parameter": { "compress-level": 1 } } } > > and back to my argument of verbosity, doesn't: > > { "compression": { "level": 1 } } } > > look better? > Yes，that will be better. >> >> EQMP >> >> { >> .name = "migrate-set-parameters", >> .args_type = >> - "compress-level:i?,compress-threads:i?,decompress-threads:i?", >> + "compress-parameter:q?", >> .mhandler.cmd_new = qmp_marshal_input_migrate_set_parameters, >> }, >> SQMP >> > > Overall, I think the idea has merit, but it will need a v2. Er, this is a 'RFC' patch, i don't know if this modification is appropriate. Since there is already a discussion about this command. I think it's better to wait for more comments before deciding whether to send v2 or not. :) Thanks, zhanghailiang
diff --git a/hmp.c b/hmp.c index e17852d..4305ae1 100644 --- a/hmp.c +++ b/hmp.c @@ -1213,27 +1213,32 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict) const char *param = qdict_get_str(qdict, "parameter"); int value = qdict_get_int(qdict, "value"); Error *err = NULL; - bool has_compress_level = false; - bool has_compress_threads = false; - bool has_decompress_threads = false; + struct MigrationCompressParameter *compress_para = + g_malloc0(sizeof(*compress_para)); + bool has_compress_parameter = false; int i; for (i = 0; i < MIGRATION_PARAMETER_MAX; i++) { if (strcmp(param, MigrationParameter_lookup[i]) == 0) { switch (i) { case MIGRATION_PARAMETER_COMPRESS_LEVEL: - has_compress_level = true; + compress_para->has_compress_level = true; + compress_para->compress_level = value; + has_compress_parameter = true; break; case MIGRATION_PARAMETER_COMPRESS_THREADS: - has_compress_threads = true; + compress_para->has_compress_threads = true; + compress_para->compress_threads = value; + has_compress_parameter = true; break; case MIGRATION_PARAMETER_DECOMPRESS_THREADS: - has_decompress_threads = true; + compress_para->has_decompress_threads = true; + compress_para->decompress_threads = value; + has_compress_parameter = true; break; } - qmp_migrate_set_parameters(has_compress_level, value, - has_compress_threads, value, - has_decompress_threads, value, + qmp_migrate_set_parameters(has_compress_parameter, + compress_para, &err); break; } diff --git a/migration/migration.c b/migration/migration.c index 732d229..eef0e5c 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -316,44 +316,49 @@ void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params, } } -void qmp_migrate_set_parameters(bool has_compress_level, - int64_t compress_level, - bool has_compress_threads, - int64_t compress_threads, - bool has_decompress_threads, - int64_t decompress_threads, Error **errp) +void qmp_migrate_set_parameters(bool has_compress_parameter, + struct MigrationCompressParameter *compress_para, + Error **errp) { MigrationState *s = migrate_get_current(); - if (has_compress_level && (compress_level < 0 || compress_level > 9)) { - error_set(errp, QERR_INVALID_PARAMETER_VALUE, "compress_level", + if (has_compress_parameter) { + if (compress_para->has_compress_level && + (compress_para->compress_level < 0 || + compress_para->compress_level > 9)) { + error_set(errp, QERR_INVALID_PARAMETER_VALUE, "compress_level", "is invalid, it should be in the range of 0 to 9"); - return; - } - if (has_compress_threads && - (compress_threads < 1 || compress_threads > 255)) { - error_set(errp, QERR_INVALID_PARAMETER_VALUE, + return; + } + if (compress_para->has_compress_threads && + (compress_para->compress_threads < 1 || + compress_para->compress_threads > 255)) { + error_set(errp, QERR_INVALID_PARAMETER_VALUE, "compress_threads", "is invalid, it should be in the range of 1 to 255"); - return; - } - if (has_decompress_threads && - (decompress_threads < 1 || decompress_threads > 255)) { - error_set(errp, QERR_INVALID_PARAMETER_VALUE, + return; + } + if (compress_para->has_decompress_threads && + (compress_para->decompress_threads < 1 || + compress_para->decompress_threads > 255)) { + error_set(errp, QERR_INVALID_PARAMETER_VALUE, "decompress_threads", "is invalid, it should be in the range of 1 to 255"); - return; - } + return; + } - if (has_compress_level) { - s->parameters[MIGRATION_PARAMETER_COMPRESS_LEVEL] = compress_level; - } - if (has_compress_threads) { - s->parameters[MIGRATION_PARAMETER_COMPRESS_THREADS] = compress_threads; - } - if (has_decompress_threads) { - s->parameters[MIGRATION_PARAMETER_DECOMPRESS_THREADS] = - decompress_threads; + if (compress_para->has_compress_level) { + s->parameters[MIGRATION_PARAMETER_COMPRESS_LEVEL] = + compress_para->compress_level; + } + if (compress_para->has_compress_threads) { + s->parameters[MIGRATION_PARAMETER_COMPRESS_THREADS] = + compress_para->compress_threads; + } + if (compress_para->has_decompress_threads) { + s->parameters[MIGRATION_PARAMETER_DECOMPRESS_THREADS] = + compress_para->decompress_threads; + } } } diff --git a/qapi-schema.json b/qapi-schema.json index 25df463..fe1b765 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -592,10 +592,8 @@ { 'enum': 'MigrationParameter', 'data': ['compress-level', 'compress-threads', 'decompress-threads'] } -# -# @migrate-set-parameters -# -# Set the following migration parameters +## +# @MigrationCompressParameter # # @compress-level: compression level # @@ -605,12 +603,24 @@ # # Since: 2.4 ## -{ 'command': 'migrate-set-parameters', +{ 'struct': 'MigrationCompressParameter', 'data': { '*compress-level': 'int', '*compress-threads': 'int', '*decompress-threads': 'int'} } # +# @migrate-set-parameters +# +# Set the migration parameters +# +# @compress-parameter: compression related parameters +# +# Since: 2.4 +## +{ 'command': 'migrate-set-parameters', + 'data': { '*compress-parameter': 'MigrationCompressParameter'} } + +# # @MigrationParameters # # @compress-level: compression level diff --git a/qmp-commands.hx b/qmp-commands.hx index 14e109e..ce721ba 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -3460,14 +3460,14 @@ Arguments: Example: -> { "execute": "migrate-set-parameters" , "arguments": - { "compress-level": 1 } } + { "compress-parameter": { "compress-level": 1 } } } EQMP { .name = "migrate-set-parameters", .args_type = - "compress-level:i?,compress-threads:i?,decompress-threads:i?", + "compress-parameter:q?", .mhandler.cmd_new = qmp_marshal_input_migrate_set_parameters, }, SQMP
'migrate-set-parameters' was specially used for setting the compression related parameters of migration. Here we re-implement it so that it can be easily extended when we want to add other parameters for migration. Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> --- hmp.c | 23 +++++++++++-------- migration/migration.c | 63 +++++++++++++++++++++++++++------------------------ qapi-schema.json | 20 ++++++++++++---- qmp-commands.hx | 4 ++-- 4 files changed, 65 insertions(+), 45 deletions(-)