diff mbox

[v2,08/54] hmp: use qapi_enum_parse() in hmp_migrate_set_capability

Message ID 20170822132255.23945-9-marcandre.lureau@redhat.com
State New
Headers show

Commit Message

Marc-André Lureau Aug. 22, 2017, 1:22 p.m. UTC
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hmp.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

Comments

Markus Armbruster Aug. 23, 2017, 9:34 a.m. UTC | #1
Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  hmp.c | 24 +++++++++++-------------
>  1 file changed, 11 insertions(+), 13 deletions(-)
>
> diff --git a/hmp.c b/hmp.c
> index 29e42ab661..4ba50e8e26 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1544,23 +1544,21 @@ void hmp_migrate_set_capability(Monitor *mon, const QDict *qdict)
>      bool state = qdict_get_bool(qdict, "state");
>      Error *err = NULL;
>      MigrationCapabilityStatusList *caps = g_malloc0(sizeof(*caps));
> -    int i;
> +    int val;
>  
> -    for (i = 0; i < MIGRATION_CAPABILITY__MAX; i++) {
> -        if (strcmp(cap, MigrationCapability_lookup[i]) == 0) {
> -            caps->value = g_malloc0(sizeof(*caps->value));
> -            caps->value->capability = i;
> -            caps->value->state = state;
> -            caps->next = NULL;
> -            qmp_migrate_set_capabilities(caps, &err);
> -            break;
> -        }
> +    val = qapi_enum_parse(MigrationCapability_lookup, cap,
> +                          MIGRATION_CAPABILITY__MAX, -1, &err);
> +    if (val < 0) {
> +        goto end;
>      }
>  
> -    if (i == MIGRATION_CAPABILITY__MAX) {
> -        error_setg(&err, QERR_INVALID_PARAMETER, cap);
> -    }
> +    caps->value = g_malloc0(sizeof(*caps->value));
> +    caps->value->capability = val;
> +    caps->value->state = state;
> +    caps->next = NULL;
> +    qmp_migrate_set_capabilities(caps, &err);
>  
> +end:
>      qapi_free_MigrationCapabilityStatusList(caps);
>  
>      if (err) {

Reviewed-by: Markus Armbruster <armbru@redhat.com>
Dr. David Alan Gilbert Aug. 24, 2017, 11:29 a.m. UTC | #2
* Marc-André Lureau (marcandre.lureau@redhat.com) wrote:
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  hmp.c | 24 +++++++++++-------------
>  1 file changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/hmp.c b/hmp.c
> index 29e42ab661..4ba50e8e26 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1544,23 +1544,21 @@ void hmp_migrate_set_capability(Monitor *mon, const QDict *qdict)
>      bool state = qdict_get_bool(qdict, "state");
>      Error *err = NULL;
>      MigrationCapabilityStatusList *caps = g_malloc0(sizeof(*caps));
> -    int i;
> +    int val;
>  
> -    for (i = 0; i < MIGRATION_CAPABILITY__MAX; i++) {
> -        if (strcmp(cap, MigrationCapability_lookup[i]) == 0) {
> -            caps->value = g_malloc0(sizeof(*caps->value));
> -            caps->value->capability = i;
> -            caps->value->state = state;
> -            caps->next = NULL;
> -            qmp_migrate_set_capabilities(caps, &err);
> -            break;
> -        }
> +    val = qapi_enum_parse(MigrationCapability_lookup, cap,
> +                          MIGRATION_CAPABILITY__MAX, -1, &err);

It would be nice to macro that to something like:
   MigrationCapability_parse(cap, -1, &err);

which would stop the inevitable case of someone using the wrong _lookup
with the wrong _MAX.

I also wonder if having the qapi_enum_parse return a bool and take a
&int would be nicer - it would get rid of the need for the -1 default
and mean actually it could be an unsigned.

Still, those are only suggestions, so:

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> +    if (val < 0) {
> +        goto end;
>      }
>  
> -    if (i == MIGRATION_CAPABILITY__MAX) {
> -        error_setg(&err, QERR_INVALID_PARAMETER, cap);
> -    }
> +    caps->value = g_malloc0(sizeof(*caps->value));
> +    caps->value->capability = val;
> +    caps->value->state = state;
> +    caps->next = NULL;
> +    qmp_migrate_set_capabilities(caps, &err);
>  
> +end:
>      qapi_free_MigrationCapabilityStatusList(caps);
>  
>      if (err) {
> -- 
> 2.14.1.146.gd35faa819
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff mbox

Patch

diff --git a/hmp.c b/hmp.c
index 29e42ab661..4ba50e8e26 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1544,23 +1544,21 @@  void hmp_migrate_set_capability(Monitor *mon, const QDict *qdict)
     bool state = qdict_get_bool(qdict, "state");
     Error *err = NULL;
     MigrationCapabilityStatusList *caps = g_malloc0(sizeof(*caps));
-    int i;
+    int val;
 
-    for (i = 0; i < MIGRATION_CAPABILITY__MAX; i++) {
-        if (strcmp(cap, MigrationCapability_lookup[i]) == 0) {
-            caps->value = g_malloc0(sizeof(*caps->value));
-            caps->value->capability = i;
-            caps->value->state = state;
-            caps->next = NULL;
-            qmp_migrate_set_capabilities(caps, &err);
-            break;
-        }
+    val = qapi_enum_parse(MigrationCapability_lookup, cap,
+                          MIGRATION_CAPABILITY__MAX, -1, &err);
+    if (val < 0) {
+        goto end;
     }
 
-    if (i == MIGRATION_CAPABILITY__MAX) {
-        error_setg(&err, QERR_INVALID_PARAMETER, cap);
-    }
+    caps->value = g_malloc0(sizeof(*caps->value));
+    caps->value->capability = val;
+    caps->value->state = state;
+    caps->next = NULL;
+    qmp_migrate_set_capabilities(caps, &err);
 
+end:
     qapi_free_MigrationCapabilityStatusList(caps);
 
     if (err) {