[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH RFC] migration: Re-implement 'migrate-set-parame
|
From: |
zhanghailiang |
|
Subject: |
Re: [Qemu-devel] [PATCH RFC] migration: Re-implement 'migrate-set-parameters' to make it easily for extension |
|
Date: |
Tue, 9 Jun 2015 19:41:30 +0800 |
|
User-agent: |
Mozilla/5.0 (Windows NT 6.1; rv:31.0) Gecko/20100101 Thunderbird/31.1.1 |
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 <address@hidden>
---
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