Patchwork [4/8] qom: push permission checks up into qdev_property_add_legacy

login
register
mail settings
Submitter Paolo Bonzini
Date Dec. 16, 2011, 12:01 p.m.
Message ID <1324036918-2405-5-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/131821/
State New
Headers show

Comments

Paolo Bonzini - Dec. 16, 2011, 12:01 p.m.
qdev_property_get and qdev_property_set can generate permission
denied errors themselves.  Do not duplicate this functionality in
qdev_get/set_legacy_property, and clean up excessive indentation.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/qdev.c |   46 +++++++++++++++++++---------------------------
 1 files changed, 19 insertions(+), 27 deletions(-)
Anthony Liguori - Dec. 16, 2011, 1:58 p.m.
On 12/16/2011 06:01 AM, Paolo Bonzini wrote:
> qdev_property_get and qdev_property_set can generate permission
> denied errors themselves.  Do not duplicate this functionality in
> qdev_get/set_legacy_property, and clean up excessive indentation.
>
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>

Nice cleanup.

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Regards,

Anthony Liguori

> ---
>   hw/qdev.c |   46 +++++++++++++++++++---------------------------
>   1 files changed, 19 insertions(+), 27 deletions(-)
>
> diff --git a/hw/qdev.c b/hw/qdev.c
> index bda8d6c..c020a6f 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -1135,46 +1135,38 @@ static void qdev_get_legacy_property(DeviceState *dev, Visitor *v, void *opaque,
>   {
>       Property *prop = opaque;
>
> -    if (prop->info->print) {
> -        char buffer[1024];
> -        char *ptr = buffer;
> +    char buffer[1024];
> +    char *ptr = buffer;
>
> -        prop->info->print(dev, prop, buffer, sizeof(buffer));
> -        visit_type_str(v,&ptr, name, errp);
> -    } else {
> -        error_set(errp, QERR_PERMISSION_DENIED);
> -    }
> +    prop->info->print(dev, prop, buffer, sizeof(buffer));
> +    visit_type_str(v,&ptr, name, errp);
>   }
>
>   static void qdev_set_legacy_property(DeviceState *dev, Visitor *v, void *opaque,
>                                        const char *name, Error **errp)
>   {
>       Property *prop = opaque;
> +    Error *local_err = NULL;
> +    char *ptr = NULL;
> +    int ret;
>
>       if (dev->state != DEV_STATE_CREATED) {
>           error_set(errp, QERR_PERMISSION_DENIED);
>           return;
>       }
>
> -    if (prop->info->parse) {
> -        Error *local_err = NULL;
> -        char *ptr = NULL;
> +    visit_type_str(v,&ptr, name,&local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>
> -        visit_type_str(v,&ptr, name,&local_err);
> -        if (!local_err) {
> -            int ret;
> -            ret = prop->info->parse(dev, prop, ptr);
> -            if (ret != 0) {
> -                error_set(errp, QERR_INVALID_PARAMETER_VALUE,
> -                          name, prop->info->name);
> -            }
> -            g_free(ptr);
> -        } else {
> -            error_propagate(errp, local_err);
> -        }
> -    } else {
> -        error_set(errp, QERR_PERMISSION_DENIED);
> +    ret = prop->info->parse(dev, prop, ptr);
> +    if (ret != 0) {
> +        error_set(errp, QERR_INVALID_PARAMETER_VALUE,
> +                  name, prop->info->name);
>       }
> +    g_free(ptr);
>   }
>
>   /**
> @@ -1194,8 +1186,8 @@ void qdev_property_add_legacy(DeviceState *dev, Property *prop,
>       type = g_strdup_printf("legacy<%s>", prop->info->name);
>
>       qdev_property_add(dev, prop->name, type,
> -                      qdev_get_legacy_property,
> -                      qdev_set_legacy_property,
> +                      prop->info->print ? qdev_get_legacy_property : NULL,
> +                      prop->info->parse ? qdev_set_legacy_property : NULL,
>                         NULL,
>                         prop, errp);
>

Patch

diff --git a/hw/qdev.c b/hw/qdev.c
index bda8d6c..c020a6f 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -1135,46 +1135,38 @@  static void qdev_get_legacy_property(DeviceState *dev, Visitor *v, void *opaque,
 {
     Property *prop = opaque;
 
-    if (prop->info->print) {
-        char buffer[1024];
-        char *ptr = buffer;
+    char buffer[1024];
+    char *ptr = buffer;
 
-        prop->info->print(dev, prop, buffer, sizeof(buffer));
-        visit_type_str(v, &ptr, name, errp);
-    } else {
-        error_set(errp, QERR_PERMISSION_DENIED);
-    }
+    prop->info->print(dev, prop, buffer, sizeof(buffer));
+    visit_type_str(v, &ptr, name, errp);
 }
 
 static void qdev_set_legacy_property(DeviceState *dev, Visitor *v, void *opaque,
                                      const char *name, Error **errp)
 {
     Property *prop = opaque;
+    Error *local_err = NULL;
+    char *ptr = NULL;
+    int ret;
 
     if (dev->state != DEV_STATE_CREATED) {
         error_set(errp, QERR_PERMISSION_DENIED);
         return;
     }
 
-    if (prop->info->parse) {
-        Error *local_err = NULL;
-        char *ptr = NULL;
+    visit_type_str(v, &ptr, name, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
 
-        visit_type_str(v, &ptr, name, &local_err);
-        if (!local_err) {
-            int ret;
-            ret = prop->info->parse(dev, prop, ptr);
-            if (ret != 0) {
-                error_set(errp, QERR_INVALID_PARAMETER_VALUE,
-                          name, prop->info->name);
-            }
-            g_free(ptr);
-        } else {
-            error_propagate(errp, local_err);
-        }
-    } else {
-        error_set(errp, QERR_PERMISSION_DENIED);
+    ret = prop->info->parse(dev, prop, ptr);
+    if (ret != 0) {
+        error_set(errp, QERR_INVALID_PARAMETER_VALUE,
+                  name, prop->info->name);
     }
+    g_free(ptr);
 }
 
 /**
@@ -1194,8 +1186,8 @@  void qdev_property_add_legacy(DeviceState *dev, Property *prop,
     type = g_strdup_printf("legacy<%s>", prop->info->name);
 
     qdev_property_add(dev, prop->name, type,
-                      qdev_get_legacy_property,
-                      qdev_set_legacy_property,
+                      prop->info->print ? qdev_get_legacy_property : NULL,
+                      prop->info->parse ? qdev_set_legacy_property : NULL,
                       NULL,
                       prop, errp);