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, &error); > 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, &error); 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); > > /* FREE 2 */ > > g_object_unref(ctx->mgr); > > > > /* FREE 9 */ > > g_free(bearer_path); > > /* FREE 1 */ > > g_free(ctx); > > > > } /* End */ > > I think you need to run valgrind to see exactly where it's leaking memory. Also, you may want to use a newer ModemManager, 1.6.4 is extremely old, not even the latest in the 1.6.x series, so leaks in libmm-glib itself may have been fixed already. -- Aleksander https://aleksander.es _______________________________________________ ModemManager-devel mailing list ModemManager-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel