qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH RFC] migration: Re-implement 'migrate-set-parameters' to make it easily for extension
@ 2015-06-09  7:18 zhanghailiang
  2015-06-09 10:49 ` Eric Blake
  0 siblings, 1 reply; 3+ messages in thread
From: zhanghailiang @ 2015-06-09  7:18 UTC (permalink / raw)
  To: qemu-devel
  Cc: zhanghailiang, quintela, armbru, liang.z.li, dgilbert,
	lcapitulino, amit.shah, peter.huangpeng

'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(-)

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
-- 
1.7.12.4

^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [Qemu-devel] [PATCH RFC] migration: Re-implement 'migrate-set-parameters' to make it easily for extension
  2015-06-09  7:18 [Qemu-devel] [PATCH RFC] migration: Re-implement 'migrate-set-parameters' to make it easily for extension zhanghailiang
@ 2015-06-09 10:49 ` Eric Blake
  2015-06-09 11:41   ` zhanghailiang
  0 siblings, 1 reply; 3+ messages in thread
From: Eric Blake @ 2015-06-09 10:49 UTC (permalink / raw)
  To: zhanghailiang, qemu-devel
  Cc: quintela, armbru, liang.z.li, dgilbert, lcapitulino, amit.shah,
	peter.huangpeng

[-- Attachment #1: Type: text/plain, Size: 8369 bytes --]

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.


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [Qemu-devel] [PATCH RFC] migration: Re-implement 'migrate-set-parameters' to make it easily for extension
  2015-06-09 10:49 ` Eric Blake
@ 2015-06-09 11:41   ` zhanghailiang
  0 siblings, 0 replies; 3+ messages in thread
From: zhanghailiang @ 2015-06-09 11:41 UTC (permalink / raw)
  To: Eric Blake, qemu-devel
  Cc: quintela, armbru, liang.z.li, peter.huangpeng, lcapitulino,
	amit.shah, dgilbert

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2015-06-09 11:50 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-06-09  7:18 [Qemu-devel] [PATCH RFC] migration: Re-implement 'migrate-set-parameters' to make it easily for extension zhanghailiang
2015-06-09 10:49 ` Eric Blake
2015-06-09 11:41   ` zhanghailiang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).