first, I commend you for going to the trouble of fixing tests in lp1316970_trusty_glib2.0.debdiff, but I think those are unrelated to this actual bug and so the patch is not required for this. I'll focus only on the first patch.
In that patch, I did a quick review of the changes, and I'm not entirely sure they are correct? LP bug comments are not a great format for reviewing patches, so please excuse formatting/line-wrapping:
this does not seem correct - per the docs, 'path' will be freed by each call in the while loop to g_variant_iter_loop(), and only needs to be manually freed after the while loop if the code breaks out of the loop manually. That doesn't appear to be the case, so path should not need freeing here. https://developer.gnome.org/glib/stable/glib-GVariant.html#g-variant-iter-loop
However value is returned from g_variant_lookup_value, which returns a value from a key-value pair in asv (as best i can tell from the docs for the function), and it does not indicate that the value is newly allocated or that it should be freed. So it does not appear that 'value' should be unref'ed here. https://developer.gnome.org/glib/stable/glib-GVariant.html#g-variant-lookup-value
Hi Seyeong,
first, I commend you for going to the trouble of fixing tests in lp1316970_ trusty_ glib2.0. debdiff, but I think those are unrelated to this actual bug and so the patch is not required for this. I'll focus only on the first patch.
In that patch, I did a quick review of the changes, and I'm not entirely sure they are correct? LP bug comments are not a great format for reviewing patches, so please excuse formatting/ line-wrapping:
> --- a/lib/services/ upstart. c upstart. c free(error) ; unref(proxy) ;
> +++ b/lib/services/
> @@ -70,6 +70,7 @@
> if (error) {
> crm_err("Can't connect obtain proxy to %s interface: %s", interface, error->message);
> g_error_
> + g_object_
this doesn't seem right - if error is non-NULL, then proxy should be NULL, according to the docs: /developer. gnome.org/ gio/stable/ GDBusProxy. html#g- dbus-proxy- new-for- bus-sync
https:/
> proxy = NULL; Upstart0_ 6.GetJobByName (in String name, out ObjectPath job) proxy_call_ sync(proxy, "GetJobByName", g_variant_ new("(s) ", arg_name), proxy_call_ sync(proxy, "GetJobByName", g_variant_ new("(s) ", arg_name), CALL_FLAGS_ NONE, -1, cancellable, error);
> }
> return proxy;
> @@ -107,7 +108,9 @@
> /*
> com.ubuntu.
> */
> - GVariant *_ret = g_dbus_
> + GVariant *_ret = NULL;
> +
> + _ret = g_dbus_
> G_DBUS_
unless i'm missing something, there is no need for this change?
> iter_free( iter); unref(_ ret);
> if (_ret) {
> @@ -200,6 +203,7 @@
>
> g_variant_
> g_variant_
> + free(path);
this does not seem correct - per the docs, 'path' will be freed by each call in the while loop to g_variant_ iter_loop( ), and only needs to be manually freed after the while loop if the code breaks out of the loop manually. That doesn't appear to be the case, so path should not need freeing here. /developer. gnome.org/ glib/stable/ glib-GVariant. html#g- variant- iter-loop
https:/
> return units;
> }
>
> @@ -224,7 +228,7 @@
> } else if (pass) {
> crm_trace("Got %s", path);
> }
> - /* free(path) */
> + free(path);
technically this should be freed only if !error - the freeing should go into the else if (pass) block above, although free(NULL) will just do nothing
however, i believe this should use g_free() instead of free()
> return pass; unref(proxy) ; unref(_ ret); unref(value) ; unref(asv) ;
> }
>
> @@ -272,6 +276,8 @@
>
> g_object_
> g_variant_
> + g_variant_
> + g_variant_
asv is returned from g_variant_ get_child_ value, which does state in its docs that the returned value should be unref'ed, so this looks correct for 'asv'. /developer. gnome.org/ glib/stable/ glib-GVariant. html#g- variant- get-child- value
https:/
However value is returned from g_variant_ lookup_ value, which returns a value from a key-value pair in asv (as best i can tell from the docs for the function), and it does not indicate that the value is newly allocated or that it should be freed. So it does not appear that 'value' should be unref'ed here. /developer. gnome.org/ glib/stable/ glib-GVariant. html#g- variant- lookup- value
https:/
> return output; get_child_ value(tmp1, 0); dup_string( tmp2, NULL); unref(tmp2) ; unref(tmp1) ;
> }
>
> @@ -299,11 +305,14 @@
> GVariant *tmp2 = g_variant_
>
> instance = g_variant_
> + g_variant_
> }
> + g_variant_
these look correct
> } unref(_ ret); unref(proxy) ;
>
> crm_info("Result: %s", instance);
> g_variant_
> + g_object_
this looks correct
> return instance;
> }
>
> @@ -338,6 +347,7 @@
> }
>
> crm_info("%s is%s running", name, pass ? "" : " not");
> + free(job);
technically this is only needed in the else {} block, but as it should still be NULL the free will do nothing
again this should be g_free() instead of free() i think
> return pass; get_type_ string( _ret),
> }
>
> @@ -400,6 +410,7 @@
> crm_info("Call to %s passed: type '%s' %s", op->action, g_variant_
> path);
> op->rc = PCMK_EXECRA_OK;
> + free(path);
looks good, but probably use g_free()
> get_type_ string( _ret)); get_type_ string( _ret),
> } else {
> crm_err("Call to %s passed but return type was '%s' not '(o)'", op->action, g_variant_
> @@ -501,6 +512,7 @@
> crm_info("Call to %s passed: type '%s' %s", op->action, g_variant_
> path);
> op->rc = PCMK_EXECRA_OK;
> + free(path);
looks good, but probably use g_free()
> get_type_ string( _ret));
> } else {
> crm_err("Call to %s passed but return type was '%s' not '(o)'", op->action, g_variant_