From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45395) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z2I2g-0001au-CS for qemu-devel@nongnu.org; Tue, 09 Jun 2015 07:50:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z2I2b-0003eK-Gr for qemu-devel@nongnu.org; Tue, 09 Jun 2015 07:50:06 -0400 Received: from szxga03-in.huawei.com ([119.145.14.66]:33936) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z2I2Y-0003Yo-Ok for qemu-devel@nongnu.org; Tue, 09 Jun 2015 07:50:01 -0400 Message-ID: <5576D0EA.7050909@huawei.com> Date: Tue, 9 Jun 2015 19:41:30 +0800 From: zhanghailiang MIME-Version: 1.0 References: <1433834286-13824-1-git-send-email-zhang.zhanghailiang@huawei.com> <5576C4C1.5090404@redhat.com> In-Reply-To: <5576C4C1.5090404@redhat.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH RFC] migration: Re-implement 'migrate-set-parameters' to make it easily for extension List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , qemu-devel@nongnu.org Cc: quintela@redhat.com, armbru@redhat.com, liang.z.li@intel.com, peter.huangpeng@huawei.com, lcapitulino@redhat.com, amit.shah@redhat.com, dgilbert@redhat.com 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 >> --- >> 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