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
> +++ b/lib/services/upstart.c
> @@ -70,6 +70,7 @@
> if (error) {
> crm_err("Can't connect obtain proxy to %s interface: %s", interface,
> error->message);
> g_error_free(error);
> + g_object_unref(proxy);
this doesn't seem right - if error is non-NULL, then proxy should be NULL,
according to the docs:
https://developer.gnome.org/gio/stable/GDBusProxy.html#g-dbus-proxy-new-for-bus-sync
> proxy = NULL;
> }
> return proxy;
> @@ -107,7 +108,9 @@
> /*
> com.ubuntu.Upstart0_6.GetJobByName (in String name, out ObjectPath job)
> */
> - GVariant *_ret = g_dbus_proxy_call_sync(proxy, "GetJobByName",
> g_variant_new("(s)", arg_name),
> + GVariant *_ret = NULL;
> +
> + _ret = g_dbus_proxy_call_sync(proxy, "GetJobByName",
> g_variant_new("(s)", arg_name),
> G_DBUS_CALL_FLAGS_NONE, -1,
> cancellable, error);
unless i'm missing something, there is no need for this change?
>
> if (_ret) {
> @@ -200,6 +203,7 @@
>
> g_variant_iter_free(iter);
> g_variant_unref(_ret);
> + 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.
https://developer.gnome.org/glib/stable/glib-GVariant.html#g-variant-iter-loop
> 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;
> }
>
> @@ -272,6 +276,8 @@
>
> g_object_unref(proxy);
> g_variant_unref(_ret);
> + g_variant_unref(value);
> + g_variant_unref(asv);
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'.
https://developer.gnome.org/glib/stable/glib-GVariant.html#g-variant-get-child-value
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
> return output;
> }
>
> @@ -299,11 +305,14 @@
> GVariant *tmp2 = g_variant_get_child_value(tmp1, 0);
>
> instance = g_variant_dup_string(tmp2, NULL);
> + g_variant_unref(tmp2);
> }
> + g_variant_unref(tmp1);
these look correct
> }
>
> crm_info("Result: %s", instance);
> g_variant_unref(_ret);
> + g_object_unref(proxy);
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;
> }
>
> @@ -400,6 +410,7 @@
> crm_info("Call to %s passed: type '%s' %s", op->action,
> g_variant_get_type_string(_ret),
> path);
> op->rc = PCMK_EXECRA_OK;
> + free(path);
looks good, but probably use g_free()
>
> } else {
> crm_err("Call to %s passed but return type was '%s' not '(o)'",
> op->action, g_variant_get_type_string(_ret));
> @@ -501,6 +512,7 @@
> crm_info("Call to %s passed: type '%s' %s", op->action,
> g_variant_get_type_string(_ret),
> path);
> op->rc = PCMK_EXECRA_OK;
> + free(path);
looks good, but probably use g_free()
>
> } else {
> crm_err("Call to %s passed but return type was '%s' not '(o)'",
> op->action, g_variant_get_type_string(_ret));
--
You received this bug notification because you are a member of Ubuntu
Touch seeded packages, which is subscribed to glib2.0 in Ubuntu.
https://bugs.launchpad.net/bugs/1316970
Title:
g_dbus memory leak in lrmd
Status in glib2.0 package in Ubuntu:
New
Status in pacemaker package in Ubuntu:
Fix Released
Status in glib2.0 source package in Trusty:
New
Status in pacemaker source package in Trusty:
In Progress
Bug description:
[Impact]
lrmd daemon with upstart resource has memory leak in Trusty
affected to pacemaker 1.1.10.
affected to glib2.0 2.40.2-0ubuntu1
Please note that patch for pacemaker is created myself.
[Test Case]
1. deploy 3 trusty instance.
2. install corosync, pacemaker, mysql.
3. setting with below info
3.1 corosync.conf, proper setting for 3 node
3.2 crm configure < setup.crm ( which has upstart:mysql setting )
3.3 monitor lrmd daemon's memory usage
[Regression]
Restarting daemon after upgrading this pkg will be needed. this patch added
NULL check for several parts. prior commit[1] changed file structure. and This
change makes user changes usage of upstart:mysql to lsb:mysql. So I added free
function myself. This might affect to system.
For glib2.0, commit [1] is critical, but [2],[3],[4] is needed for building
it.
[Others]
Related commits.
[1] commit a7b61e276120184c7586a3217ed3571a982f5017
Author: Andrew Beekhof <and...@beekhof.net>
Date: Fri Aug 23 16:25:35 2013 +1000
Refactor: attrd: Move to its own directory and create a stub for
attrd-ng
------------------------------------------------------------------
$ git describe --contains a1a6922e43dfe80b23887a88401cbb93fe3645c0
Pacemaker-1.1.11-rc3
$ git describe --contains a7b61e276120184c7586a3217ed3571a982f5017
Pacemaker-1.1.11-rc1~168
$ rmadison pacemaker
pacemaker | 1.1.10+git20130802-1ubuntu2 | trusty
pacemaker | 1.1.10+git20130802-1ubuntu2.4 | trusty-security
pacemaker | 1.1.10+git20130802-1ubuntu2.4 | trusty-updates
pacemaker | 1.1.14-2ubuntu1 | xenial
pacemaker | 1.1.14-2ubuntu1.3 | xenial-security
pacemaker | 1.1.14-2ubuntu1.3 | xenial-update
pacemaker | 1.1.16-1ubuntu1 | zesty
pacemaker | 1.1.16-1ubuntu1 | artful
pacemaker | 1.1.18~rc3-1ubuntu1 | bionic
------------------------------------------------------------------
For glib
[1]
https://github.com/GNOME/glib/commit/db641e32920ee8b553ab6f2d318aafa156e4390c
[2]
https://github.com/GNOME/glib/commit/8792609e15394967cab526838b83f90acb401663
[3]
https://github.com/GNOME/glib/commit/ec02a1875f29ecb8e46c0d8c1403cd00a0b3a9e4
[4]
https://github.com/GNOME/glib/commit/f10b6550ff2ce55d06b92d6dc3e443fc007b2f7a
[Original Description]
I'm running Pacemaker 1.1.10+git20130802-1ubuntu1 on Ubuntu Saucy
(13.10) and have encountered a memory leak in lrmd.
The details of the bug are covered here in this thread
(http://oss.clusterlabs.org/pipermail/pacemaker/2014-May/021689.html)
but to summarise, the Pacemaker developers believe the leak is caused
by the g_dbus API, the use of which was removed in Pacemaker 1.11.
I've also attached the Valgrind output from the run that exposed the
issue.
Given that this issue affects production stability (a periodic restart
of Pacemaker is required), will a version of 1.11 be released for
Trusty? (I'm happy to upgrade the OS to Trusty to get it).
If not, can you advise which version of the OS will be the first to
take 1.11 please?
To manage notifications about this bug go to:
https://bugs.launchpad.net/ubuntu/+source/glib2.0/+bug/1316970/+subscriptions
--
Mailing list: https://launchpad.net/~touch-packages
Post to : touch-packages@lists.launchpad.net
Unsubscribe : https://launchpad.net/~touch-packages
More help : https://help.launchpad.net/ListHelp
