Merge lp:~charlesk/libindicate/lp-957611 into lp:libindicate/0.7

[Charles Kerr]

[Charles Kerr (charlesk)]

Before:

  if (error) {
    g_warning ("error!");
    goto cleanup;
  }
  GVariant foo = bar; /* Coverity is unhappy about foo + goto */
  yadda;
cleanup:
   ...

After:

  if (error) {
    g_warning ("error!");
  } else {
    GVariant foo = bar;
    yadda;
  }
  ...

[Lars Karlitski (larsu)]

Has nothing to do with the intention of the patch, but 'value' is still leaked (line 56 in the diff).

review: Needs Fixing

[Charles Kerr (charlesk)]

Thank you lars!

[lp:~charlesk/libindicate/lp-957611]

451. By Charles Kerr on 2012-03-19

fix pre-existing GVariant leak that my patch didn't fix. reported by larsu

[Lars Karlitski (larsu)]

Excellent, thank you Charles!

review: Approve

=== modified file 'libindicate/listener.c'
--- libindicate/listener.c	2012-02-09 23:17:59 +0000
+++ libindicate/listener.c	2012-03-19 20:33:20 +0000
@@ -958,52 +958,80 @@
 	if (error != NULL) {
 		g_warning("Unable to get property data: %s", error->message);
 		g_error_free(error);
-		goto cleanup;
-	}
-
-	GVariant * value = g_variant_get_child_value(retvalue, 0);
-	if (g_variant_is_of_type(value, G_VARIANT_TYPE_VARIANT)) {
-		value = g_variant_get_variant(value);
-	}
-
-	switch (get_property_data->type) {
-	case PROPERTY_TYPE_VARIANT: {
-		/* Just pass the GVariant along. */
-		indicate_listener_get_property_variant_cb cb =(indicate_listener_get_property_variant_cb)get_property_data->cb;
-		cb(get_property_data->listener, get_property_data->server, get_property_data->indicator, get_property_data->property, value, get_property_data->data);
-		break;
-	}
-	case PROPERTY_TYPE_STRING: {
-		/* Just pass the string along. */
-		indicate_listener_get_property_cb cb = (indicate_listener_get_property_cb)get_property_data->cb;
-		cb(get_property_data->listener, get_property_data->server, get_property_data->indicator, get_property_data->property, g_variant_get_string(value, NULL), get_property_data->data);
-		break;
-	}
-	case PROPERTY_TYPE_TIME: {
-		/* Convert it to a time val */
-		indicate_listener_get_property_time_cb cb = (indicate_listener_get_property_time_cb)get_property_data->cb;
-		GTimeVal time;
-		if (g_time_val_from_iso8601(g_variant_get_string(value, NULL), &time)) {
-			cb(get_property_data->listener, get_property_data->server, get_property_data->indicator, get_property_data->property, &time, get_property_data->data);
-		}
-		break;
-	}
-	case PROPERTY_TYPE_INT: {
-		/* Take the string and convert it to an integer */
-		indicate_listener_get_property_int_cb cb = (indicate_listener_get_property_int_cb)get_property_data->cb;
-		cb(get_property_data->listener, get_property_data->server, get_property_data->indicator, get_property_data->property, g_variant_get_int32(value), get_property_data->data);
-		break;
-	}
-	case PROPERTY_TYPE_BOOL: {
-		/* Check to see if it's 'true', if not assume that
-		   it's false */
-		indicate_listener_get_property_bool_cb cb = (indicate_listener_get_property_bool_cb)get_property_data->cb;
-		cb(get_property_data->listener, get_property_data->server, get_property_data->indicator, get_property_data->property, g_variant_get_boolean(value), get_property_data->data);
-		break;
-	}
-	}
-
-	cleanup:
+	} else {
+
+		GVariant * value = g_variant_get_child_value(retvalue, 0);
+		if (g_variant_is_of_type(value, G_VARIANT_TYPE_VARIANT)) {
+			GVariant * tmp = g_variant_get_variant(value);
+			g_variant_unref (value);
+			value = tmp;
+		}
+
+		switch (get_property_data->type) {
+		case PROPERTY_TYPE_VARIANT: {
+			/* Just pass the GVariant along. */
+			indicate_listener_get_property_variant_cb cb =(indicate_listener_get_property_variant_cb)get_property_data->cb;
+			cb(get_property_data->listener,
+			   get_property_data->server,
+			   get_property_data->indicator,
+			   get_property_data->property,
+			   value,
+			   get_property_data->data);
+			break;
+		}
+		case PROPERTY_TYPE_STRING: {
+			/* Just pass the string along. */
+			indicate_listener_get_property_cb cb = (indicate_listener_get_property_cb)get_property_data->cb;
+			cb(get_property_data->listener,
+			   get_property_data->server,
+			   get_property_data->indicator,
+			   get_property_data->property,
+			   g_variant_get_string(value, NULL),
+			   get_property_data->data);
+			break;
+		}
+		case PROPERTY_TYPE_TIME: {
+			/* Convert it to a time val */
+			indicate_listener_get_property_time_cb cb = (indicate_listener_get_property_time_cb)get_property_data->cb;
+			GTimeVal time;
+			if (g_time_val_from_iso8601(g_variant_get_string(value, NULL), &time)) {
+				cb(get_property_data->listener,
+				   get_property_data->server,
+				   get_property_data->indicator,
+				   get_property_data->property,
+				   &time,
+				   get_property_data->data);
+			}
+			break;
+		}
+		case PROPERTY_TYPE_INT: {
+			/* Take the string and convert it to an integer */
+			indicate_listener_get_property_int_cb cb = (indicate_listener_get_property_int_cb)get_property_data->cb;
+			cb(get_property_data->listener,
+			   get_property_data->server,
+			   get_property_data->indicator,
+			   get_property_data->property,
+			   g_variant_get_int32(value),
+			   get_property_data->data);
+			break;
+		}
+		case PROPERTY_TYPE_BOOL: {
+			/* Check to see if it's 'true', if not assume that
+			   it's false */
+			indicate_listener_get_property_bool_cb cb = (indicate_listener_get_property_bool_cb)get_property_data->cb;
+			cb(get_property_data->listener,
+			   get_property_data->server,
+			   get_property_data->indicator,
+			   get_property_data->property,
+			   g_variant_get_boolean(value),
+			   get_property_data->data);
+			break;
+		}
+		}
+
+		g_variant_unref (value);
+	}
+
 	if (retvalue != NULL) {
 		g_variant_unref(retvalue);
 	}