Re: [PATCH v2 2/5] domain: replace bool accel{2d, 3d} with a tristate

[Michal Privoznik <mprivozn@xxxxxxxxxx>]

On 25.11.2015 09:42, Marc-André Lureau wrote:
> Allowing to have the extra undefined/default state.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@xxxxxxxxx>
> ---
>  src/conf/domain_conf.c | 41 ++++++++++++++++++++++++++---------------
>  src/conf/domain_conf.h |  4 ++--
>  src/vbox/vbox_common.c | 18 ++++++++++++------
>  3 files changed, 40 insertions(+), 23 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index f501f14..111c2ae 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -11961,8 +11961,9 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node)
>  {
>      xmlNodePtr cur;
>      virDomainVideoAccelDefPtr def;
> -    char *accel3d = NULL;
>      char *accel2d = NULL;
> +    char *accel3d = NULL;
> +    int val;

This @val variable is not much useful, since def->accel{2,3}d is already type of int, but doesn't hurt to have it here too. I bet compiler will optimize it out anyway. And I don't want to be too picky too.

>  
>      cur = node->children;
>      while (cur != NULL) {
> @@ -11983,21 +11984,26 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node)
>          return NULL;

There's another bug in here, just above this return NULL ^^^ there are two allocations, if the first succeeds but the second fails, we leak the first allocation. So this return NULL should be replaced with goto cleanup.

>  
>      if (accel3d) {
> -        if (STREQ(accel3d, "yes"))
> -            def->accel3d = true;
> -        else
> -            def->accel3d = false;
> -        VIR_FREE(accel3d);
> +        if ((val = virTristateBoolTypeFromString(accel3d)) <= 0) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                           _("unknown accel3d value '%s'"), accel3d);
> +            goto end;
> +        }
> +        def->accel3d = val;
>      }
>  
>      if (accel2d) {
> -        if (STREQ(accel2d, "yes"))
> -            def->accel2d = true;
> -        else
> -            def->accel2d = false;
> -        VIR_FREE(accel2d);
> +        if ((val = virTristateBoolTypeFromString(accel2d)) <= 0) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                           _("unknown accel2d value '%s'"), accel2d);
> +            goto end;
> +        }
> +        def->accel2d = val;
>      }
>  
> +end:

s/^/ / it's our coding style. Then, we tend to use cleanup as a name for labels rather than end.

> +    VIR_FREE(accel2d);
> +    VIR_FREE(accel3d);
>      return def;
>  }
>  
> @@ -20837,10 +20843,15 @@ static void
>  virDomainVideoAccelDefFormat(virBufferPtr buf,
>                               virDomainVideoAccelDefPtr def)
>  {
> -    virBufferAsprintf(buf, "<acceleration accel3d='%s'",
> -                      def->accel3d ? "yes" : "no");
> -    virBufferAsprintf(buf, " accel2d='%s'",
> -                      def->accel2d ? "yes" : "no");
> +    virBufferAsprintf(buf, "<acceleration");

For adding literal strings to a buffer we have a special function: virBufferAddLit().

> +    if (def->accel3d) {
> +        virBufferAsprintf(buf, " accel3d='%s'",
> +                          virTristateBoolTypeToString(def->accel3d));
> +    }
> +    if (def->accel2d) {
> +        virBufferAsprintf(buf, " accel2d='%s'",
> +                          virTristateBoolTypeToString(def->accel2d));
> +    }
>      virBufferAddLit(buf, "/>\n");
>  }


Right, I went through all occurrences of accel{2,3}d and you are fixing all the places needed.

ACK with this squashed in:

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index e095115..e04227a 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -12027,13 +12027,13 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node)
         return NULL;
 
     if (VIR_ALLOC(def) < 0)
-        return NULL;
+        goto cleanup;
 
     if (accel3d) {
         if ((val = virTristateBoolTypeFromString(accel3d)) <= 0) {
             virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
                            _("unknown accel3d value '%s'"), accel3d);
-            goto end;
+            goto cleanup;
         }
         def->accel3d = val;
     }
@@ -12042,12 +12042,12 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node)
         if ((val = virTristateBoolTypeFromString(accel2d)) <= 0) {
             virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
                            _("unknown accel2d value '%s'"), accel2d);
-            goto end;
+            goto cleanup;
         }
         def->accel2d = val;
     }
 
-end:
+ cleanup:
     VIR_FREE(accel2d);
     VIR_FREE(accel3d);
     return def;
@@ -20879,7 +20879,7 @@ static void
 virDomainVideoAccelDefFormat(virBufferPtr buf,
                              virDomainVideoAccelDefPtr def)
 {
-    virBufferAsprintf(buf, "<acceleration");
+    virBufferAddLit(buf, "<acceleration");
     if (def->accel3d) {
         virBufferAsprintf(buf, " accel3d='%s'",
                           virTristateBoolTypeToString(def->accel3d));

Michal

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list