From 2991bc71f36035cc85f7b6246d7c114a3961b5db Mon Sep 17 00:00:00 2001 From: Alex Wilson Date: Wed, 26 Feb 2020 08:42:41 -0700 Subject: [PATCH 1/5] battstat: call up_client_get_devices once to avoid mem leak This attempts to avoid a memory leak when calling up_client_get_devices() from libupower-glib by only calling it once in battstat_upower_initialise() and reusing that GPtrArray later in battstat_upower_get_battery_info(). This leak was discovered by running strace for mmap and brk syscalls. No calls to mmap were made, but the brk could caught and backtraced over and over again to battstat_upower_get_battery_info(). There may be some issues with UP_CHECK_VERSION (0, 99, 0) checks that need to be addressed here that are made in battstat_upower_initialise(), but not in battstat_upower_get_battery_info(). See 'Memory leak of battstat-applet #345' for more details: https://github.com/mate-desktop/mate-applets/issues/345 --- battstat/battstat-upower.c | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/battstat/battstat-upower.c b/battstat/battstat-upower.c index 67779f8c3..a1e545df4 100644 --- a/battstat/battstat-upower.c +++ b/battstat/battstat-upower.c @@ -43,6 +43,8 @@ static void (*status_updated_callback) (void); */ static gboolean status_update_scheduled; +static GPtrArray *devices; + static gboolean update_status_idle (gpointer junk) { @@ -83,9 +85,6 @@ battstat_upower_initialise (void (*callback) (void)) int i, num; status_updated_callback = callback; -#if UP_CHECK_VERSION (0, 99, 0) - GPtrArray *devices; -#endif if( upc != NULL ) return g_strdup( "Already initialised!" ); @@ -101,7 +100,6 @@ battstat_upower_initialise (void (*callback) (void)) if (!devices) { goto error_shutdownclient; } - g_ptr_array_unref(devices); #else if (! up_client_enumerate_devices_sync( upc, cancellable, &gerror ) ) { sprintf(error_str, "Unable to enumerate upower devices: %s\n", gerror->message); @@ -132,7 +130,7 @@ battstat_upower_cleanup( void ) { if( upc == NULL ) return; - + g_object_unref( upc ); upc = NULL; } @@ -154,9 +152,6 @@ battstat_upower_cleanup( void ) void battstat_upower_get_battery_info( BatteryStatus *status ) { - - GPtrArray *devices = up_client_get_devices( upc ); - /* The calculation to get overall percentage power remaining is as follows: * * Sum( Current charges ) / Sum( Full Capacities ) @@ -211,7 +206,7 @@ battstat_upower_get_battery_info( BatteryStatus *status ) int type, state; double current_charge, full_capacity, rate; gint64 time_to_full, time_to_empty; - + g_object_get( upd, "kind", &type, "state", &state, @@ -259,7 +254,6 @@ battstat_upower_get_battery_info( BatteryStatus *status ) status->on_ac_power = TRUE; status->charging = FALSE; - g_ptr_array_unref( devices ); return; } @@ -279,7 +273,7 @@ battstat_upower_get_battery_info( BatteryStatus *status ) * (ie: the PMU or APM subsystem). * * upower gives remaining time in seconds with a 0 to mean that the - * remaining time is unknown. Battstat uses minutes and -1 for + * remaining time is unknown. Battstat uses minutes and -1 for * unknown time remaining. */ @@ -326,8 +320,6 @@ battstat_upower_get_battery_info( BatteryStatus *status ) /* These are simple and well-explained above. */ status->charging = charging; status->on_ac_power = on_ac_power; - - g_ptr_array_unref( devices ); } void From a73baf2aedfa90aece1b446ff6157573d897a59d Mon Sep 17 00:00:00 2001 From: Alex Wilson Date: Wed, 26 Feb 2020 09:04:24 -0700 Subject: [PATCH 2/5] battstat: remove whitespace noise; free devices ptr in cleanup func --- battstat/battstat-upower.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/battstat/battstat-upower.c b/battstat/battstat-upower.c index a1e545df4..eb1d1a146 100644 --- a/battstat/battstat-upower.c +++ b/battstat/battstat-upower.c @@ -130,7 +130,8 @@ battstat_upower_cleanup( void ) { if( upc == NULL ) return; - + + g_object_unref( devices ); g_object_unref( upc ); upc = NULL; } @@ -206,7 +207,7 @@ battstat_upower_get_battery_info( BatteryStatus *status ) int type, state; double current_charge, full_capacity, rate; gint64 time_to_full, time_to_empty; - + g_object_get( upd, "kind", &type, "state", &state, @@ -273,7 +274,7 @@ battstat_upower_get_battery_info( BatteryStatus *status ) * (ie: the PMU or APM subsystem). * * upower gives remaining time in seconds with a 0 to mean that the - * remaining time is unknown. Battstat uses minutes and -1 for + * remaining time is unknown. Battstat uses minutes and -1 for * unknown time remaining. */ From 591ebcc7764124efcf78f589df025072d50c6868 Mon Sep 17 00:00:00 2001 From: Alex Wilson Date: Wed, 26 Feb 2020 09:44:10 -0700 Subject: [PATCH 3/5] battstat: use more specific unref for devices ptr; rearrange cleanup --- battstat/battstat-upower.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/battstat/battstat-upower.c b/battstat/battstat-upower.c index eb1d1a146..5d5a1d5bb 100644 --- a/battstat/battstat-upower.c +++ b/battstat/battstat-upower.c @@ -128,12 +128,13 @@ battstat_upower_initialise (void (*callback) (void)) void battstat_upower_cleanup( void ) { - if( upc == NULL ) - return; + if( upc != NULL ) + g_object_unref( upc ); + if( devices != NULL ) + g_ptr_array_unref( devices ); - g_object_unref( devices ); - g_object_unref( upc ); upc = NULL; + devices = NULL; } #include "battstat.h" From 135c7c4f495fa40aa290a7df824a2a159cc101d8 Mon Sep 17 00:00:00 2001 From: Alex Wilson Date: Wed, 26 Feb 2020 10:21:45 -0700 Subject: [PATCH 4/5] battstat: bump upower version; update depricated funcs and remove upower version checks --- battstat/battstat-upower.c | 16 +--------------- configure.ac | 2 +- 2 files changed, 2 insertions(+), 16 deletions(-) diff --git a/battstat/battstat-upower.c b/battstat/battstat-upower.c index 5d5a1d5bb..533b6d0cb 100644 --- a/battstat/battstat-upower.c +++ b/battstat/battstat-upower.c @@ -69,12 +69,10 @@ device_cb (UpClient *client, UpDevice *device, gpointer user_data) { schedule_status_callback(); } -#if UP_CHECK_VERSION (0, 99, 0) static void device_removed_cb (UpClient *client, const gchar *object_path, gpointer user_data) { schedule_status_callback(); } -#endif /* ---- public functions ---- */ @@ -95,25 +93,13 @@ battstat_upower_initialise (void (*callback) (void)) GCancellable *cancellable = g_cancellable_new(); GError *gerror; -#if UP_CHECK_VERSION(0, 99, 0) - devices = up_client_get_devices(upc); + devices = up_client_get_devices2(upc); if (!devices) { goto error_shutdownclient; } -#else - if (! up_client_enumerate_devices_sync( upc, cancellable, &gerror ) ) { - sprintf(error_str, "Unable to enumerate upower devices: %s\n", gerror->message); - goto error_shutdownclient; - } -#endif g_signal_connect_after( upc, "device-added", G_CALLBACK (device_cb), NULL ); -#if UP_CHECK_VERSION(0, 99, 0) g_signal_connect_after( upc, "device-removed", G_CALLBACK (device_removed_cb), NULL ); -#else - g_signal_connect_after( upc, "device-changed", G_CALLBACK (device_cb), NULL ); - g_signal_connect_after( upc, "device-removed", G_CALLBACK (device_cb), NULL ); -#endif return NULL; diff --git a/configure.ac b/configure.ac index b08f655ce..ff2bf4ca0 100644 --- a/configure.ac +++ b/configure.ac @@ -18,7 +18,7 @@ GTK_REQUIRED=3.22.0 LIBPANEL4_REQUIRED=1.17.0 LIBGTOP_REQUIRED=2.12.0 LIBNOTIFY_REQUIRED=0.7.0 -UPOWER_REQUIRED=0.9.4 +UPOWER_REQUIRED=0.99.8 DBUS_REQUIRED=1.10.0 DBUS_GLIB_REQUIRED=0.74 LIBXML_REQUIRED=2.5.0 From be4387241994d8166857d8e0a18c7dedf0023b29 Mon Sep 17 00:00:00 2001 From: Alex Wilson Date: Wed, 26 Feb 2020 11:09:19 -0700 Subject: [PATCH 5/5] add autoconf-archive pkg to arch build config --- .build.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.build.yml b/.build.yml index 8d6dd49a8..e600468ea 100644 --- a/.build.yml +++ b/.build.yml @@ -21,6 +21,7 @@ requires: - which - wireless_tools - yelp-tools + - autoconf-archive debian: # Useful URL: https://github.com/mate-desktop/debian-packages