Hey,
> I am using the libmm-glib API to query bearer status periodically and
> synchronously from MM(v1.6.4) from my own linux process. I have used mmcli
> as an example as was suggested on this forum previously.
>
> (https://lists.freedesktop.org/archives/modemmanager-devel/2017-September/005700.html)
>
> After few queries of bearer status, I see my process leaking memory. I think
> I am freeing all the allocations using g_object_unref and g_free but not
> sure what I am leaving behind.
>
> I am not very familiar with glib memory management routines or with
> libmm-glib internals and so I am asking for help here hoping someone can
> give me some direction.
>
Run your program as:
$ G_SLICE=always-malloc valgrind --leak-check=full your-program .
And then look at the "definitely lost" memory leak blocks valgrind
reports. You'll see exactly where the leaks are happening.
Anyway, see below more commetns.
>
>
> The following function is called periodically to get the bearer object(to
> read bearer connection status) and I have marked the object memory
> allocations and frees. I think I’m freeing all the allocations yet I see my
> process leaking memory due to calling this function.
>
> Is there something obviously wrong? Is there a better way to query bearer
> connectivity status from MM?
>
Yes, there is something wrong, why are you periodically doing all
those checks!?? :)
You could do it once and rely on asynchronous "real time" updates
happening on the background.
>
>
> func_get_bearer() {
>
>
>
> /* ALLOC 1 */
>
> ctx = g_new0(Context, 1); /* Context a struct which has MMManager MMObject,
> MMMOdem etc. */
>
>
>
> /* ALLOC 2 */
>
> mgr = mm_manager_new_sync(connection_object,
>
>
> G_DBUS_OBJECT_MANAGER_CLIENT_FLAGS_DO_NOT_AUTO_START,
>
> NULL, );
>
If mgr is NULL, error is set. You'd need to catch that and
g_error_free() the GError.
>
>
> /* ALLOC 3 */
>
> name =
> g_dbus_object_manager_client_get_name_owner(G_DBUS_OBJECT_MANAGER_CLIENT(mgr));
>
> if(name == NULL)
>
> {
>
>g_object_unref(mgr);
>
>/* Error */
>
>return;
>
> }
>
> /* FREE 3 */
>
> g_free(name);
>
>
>
> /* Get modem path */
>
> /* ALLOC 4 */
>
> modem_path = g_strdup_printf(MM_DBUS_MODEM_PREFIX "/%s", modem_id); /*
> modem_id may be “0” */
>
This is not ok, you cannot just rely on a specific modem index. If
the modem resets itself it will get a new index, it won't be 0.
>
>
> /* Get modem object */
>
> /* ALLOC 5 */
>
> modems = g_dbus_object_manager_get_objects(G_DBUS_OBJECT_MANAGER(mgr));
>
> for(mdm = modems; mdm; mdm = g_list_next(mdm))
>
> {
>
>mdm = MM_OBJECT(mdm->data);
Wait, this is not right. "mdm" is what? "mdm" was a GList (I assume)
because you use it to iterate the "modems" list, you shouldn't re-use
it to cast mdm->data because then the list iteration will break.
You could do:
MMObject *iter = MM_OBJECT(mdm->data);
>
>if((modem_path != NULL) && g_str_equal(mm_object_get_path(mm_obj),
> modem_path))
>
>{
As said, don't compare from the list against a predefined index, that
is not right, won't work as you expect. If you only expect one modem
in your system, as soon as you find a modem reported, that is the one
you want, regardless of whether it's index 0 or 1 or whatever.
>
> /* ALLOC 6 */
>
> found = g_object_ref(mm_obj);
>
> break;
>
>}
>
> }
>
> /* FREE 5 */
>
> g_list_free_full(modems, g_object_unref);
>
> /* FREE 4 */
>
> g_free(modem_path);
>
>
>
> ctx->m_generic_object = found;
>
>
>
> /* ALLOC 7 */
>
> ctx->mdm_intferface_obj = mm_object_get_modem(ctx->m_generic_object);
>
>
>
> /* Get list of bearers */
>
> /* ALLOC 8 */
>
> b_list = mm_modem_list_bearers_sync(ctx->mdm_intferface_obj, NULL, );
If b_list is NULL, error is set, so you would need to catch that and
g_error_free the GError.
>
>
>
> /* Construct bearer path */
>
> /* ALLOC 9 */
>
> bearer_path = g_strdup_printf(MM_DBUS_BEARER_PREFIX "/%s", path_or_idx);
>
This is also not needed at all, don't match against a specific index again.
>
>
> /* Get bearer object */
>
> GList *bearer;
>
> MMBearer *bearer_obj;
>
>
>
> for(bearer = b_list; bearer; bearer = g_list_next(bearer))
>
> {
>
>bearer_obj = MM_BEARER(bearer->data);
>
>if(bearer_obj == NULL)
>
>{
>
> continue;
>
>}
This previous if can never happen. The list of bearers will always
have a valid data.
>
>if(g_str_equal(mm_bearer_get_path(bearer_obj), bearer_path))
>
>{
As said earlier, don't match, that's wrong. You can just rely on "if I
find one bearer, that's the one I want".
>
> /* Got bearer object */
>
> /* ALLOC 10 */
>
> ctx->bearer_obj = g_object_ref(bearer_obj);
>
>}
>
> }
>
>
>
> /* Free the allocations */
>
> /* FREE 8 */
>
> g_list_free_full(b_list, g_object_unref);
>
>
>
> /* FREE 10 */
>
> g_object_unref(ctx->bearer_obj);
>
> /* FREE 6 */
>
> g_object_unref(ctx->m_generic_object);
>
> /* FREE 7 */
>
> g_object_unref(ctx->mdm_intferface_obj);
>
>
