NM plugin fixes #13

wentasah · 2018-12-16T08:48:19Z

Hi,

the main goal of this PR is to fix an issue described in commit nm: Always refresh SSID after suspend/resume cycle. The other commits fix minor issues that I encountered during debugging the problem.

This fixes the following warning: warning: assignment to ‘const GByteArray *’ {aka ‘const struct _GByteArray *’} from incompatible pointer type ‘GBytes *’ {aka ‘struct _GBytes *’} [-Wincompatible-pointer-types] Signed-off-by: Michal Sojka <michal.sojka@cvut.cz>

The documentation [1] says that conversion of SSID to human readable string should be done only with this function. [1] https://developer.gnome.org/libnm-util/stable/libnm-util-nm-utils.html#nm-utils-ssid-to-utf8

Signed-off-by: Michal Sojka <michal.sojka@cvut.cz>

This ensures that when _j4status_nm_device_update() is called again but data.ssid is not set to a new value, we do not unreference possibly already freed variable.

When suspending a laptop and resuming it at place with different Wi-Fi AP, the SSID was not updated, i.e. the SSID of where the laptop was suspended was shown all the time even when Wi-Fi connected to another AP after resume. This was caused by not updating section->ap to the new AP after resuming. It was updated only after AP has changed while j4status was running. The problem could be reproduced this way: 1) Have two active APs that network manager (NM) can connect to. 2) Manually connect to the first AP. 3) Send j4status SIGUSR2 to stop it. 4) Manually connect to second AP. 5) Send j4status SIGUSR1 to let it continue (start). 6) Observe that j4status prints SSID of the first AP instead of the second one. This is fixed by always setting section->ap in _j4status_nm_device_monitor(), which is called from start callback (after resume). We also fix some reference counting problems.

sardemff7

I’m not sure we have a NetworkManager/libnm bug here, suspend is a bit tricky.
Anyway, this is a nice set of fixes, though with a bunch of style issues (and some over-cleanup maybe?)

I have one question about the last commit: why are you saying “We also fix some reference counting problems”?
Is that about ap? Before your patch, it’s ref’ed at the start and when it changes, and unref’ed at the stop and when it changes. Did you have any pre-patch issue?
Is that about data.ssid? If so, please make a different commit and explain fully the reason you’ve hit this issue.

I’ll gladly merge all that once the changes are made :-)

sardemff7 · 2018-12-16T11:04:10Z

nm/src/nm.c

                g_variant_unref(data.ssid);
+                data.ssid = NULL;
+            }


data is a local variable and will not survive the case block so there is no need to set it to NULL.
Did you hit any issue here? If so, it’s a good thing to say it in the commit message and explain why.
It would definitely need its own commit if you really need that.

sardemff7 · 2018-12-16T11:06:27Z

nm/src/nm.c

@@ -494,8 +494,10 @@ _j4status_nm_device_property_changed(NMDevice *device, GParamSpec *pspec, gpoint
            g_object_unref(section->ap);
        }
        section->ap = nm_device_wifi_get_active_access_point(NM_DEVICE_WIFI(device));
-        if ( section->ap != NULL )
+        if ( section->ap != NULL ) {
+            g_object_ref(section->ap);
            section->strength_handler = g_signal_connect(g_object_ref(section->ap), "notify::strength", G_CALLBACK(_j4status_nm_access_point_property_changed), section);


Here the g_object_ref() is done already. You need to stick to one, don’t mind either, though I used inline one to avoid bracket back then.

sardemff7 · 2018-12-16T11:11:56Z

nm/src/nm.c

-                    const GByteArray *raw_ssid;
-                    raw_ssid = nm_access_point_get_ssid(section->ap);
-                    data.ssid = g_variant_new_take_string(g_strndup((const gchar *)raw_ssid->data, raw_ssid->len));
+                    GBytes *raw_ssid = nm_access_point_get_ssid(section->ap);


Hum, that’s new from the libnm merge, so you need to say that in the commit message.

sardemff7 · 2018-12-16T11:12:58Z

nm/src/nm.c

-                    data.ssid = g_variant_new_take_string(g_strndup((const gchar *)raw_ssid->data, raw_ssid->len));
+                    GBytes *raw_ssid = nm_access_point_get_ssid(section->ap);
+                    data.ssid = g_variant_new_take_string(g_strndup(g_bytes_get_data(raw_ssid, NULL),
+                                                                    g_bytes_get_size(raw_ssid)));


Nitpick: please do not change the style of the code. Here, just make it one (not event that long) line

sardemff7 · 2018-12-16T11:14:50Z

nm/src/nm.c

@@ -404,8 +404,11 @@ _j4status_nm_device_update(J4statusPluginContext *context, J4statusNmSection *se
                if ( context->formats.up_wifi_tokens & TOKEN_FLAG_UP_WIFI_SSID )
                {
                    GBytes *raw_ssid = nm_access_point_get_ssid(section->ap);
-                    data.ssid = g_variant_new_take_string(g_strndup(g_bytes_get_data(raw_ssid, NULL),
-                                                                    g_bytes_get_size(raw_ssid)));
+                    if (raw_ssid) {


This condition must be in the previous commit as it’s the one introducing the NULL case.

Nit: also 3 style issues here: spaces around parens, != NULL and bracket on the following line

sardemff7 · 2018-12-16T11:15:49Z

nm/src/nm.c

-_j4status_nm_format_down_other_callback(const gchar *token, guint64 value, gconstpointer user_data)
+_j4status_nm_format_down_other_callback(G_GNUC_UNUSED const gchar *token,
+                                        G_GNUC_UNUSED guint64 value,
+                                        G_GNUC_UNUSED gconstpointer user_data)


Nit: style, all on the same line

sardemff7 · 2018-12-16T11:16:00Z

nm/src/nm.c

@@ -471,7 +473,8 @@ _j4status_nm_device_update(J4statusPluginContext *context, J4statusNmSection *se
 }

 static void
-_j4status_nm_access_point_property_changed(NMAccessPoint *device, GParamSpec *pspec, gpointer user_data)
+_j4status_nm_access_point_property_changed(G_GNUC_UNUSED NMAccessPoint *device,
+                                           G_GNUC_UNUSED GParamSpec *pspec, gpointer user_data)


sardemff7 · 2018-12-16T11:16:05Z

nm/src/nm.c

@@ -496,14 +499,17 @@ _j4status_nm_device_property_changed(NMDevice *device, GParamSpec *pspec, gpoint
 }

 static void
-_j4status_nm_device_state_changed(NMDevice *device, guint state, guint arg2, guint arg3, gpointer user_data)
+_j4status_nm_device_state_changed(NMDevice *device,


sardemff7 · 2018-12-16T11:22:54Z

nm/src/nm.c

@@ -523,8 +525,11 @@ _j4status_nm_device_monitor(G_GNUC_UNUSED gpointer key, gpointer data, G_GNUC_UN
    case NM_DEVICE_TYPE_WIFI:
        section->bitrate_handler = g_signal_connect(section->device, "notify::bitrate", G_CALLBACK(_j4status_nm_device_property_changed), section);
        section->active_access_point_handler = g_signal_connect(section->device, "notify::active-access-point", G_CALLBACK(_j4status_nm_device_property_changed), section);
-        if ( section->ap != NULL )
+        section->ap = nm_device_wifi_get_active_access_point(NM_DEVICE_WIFI(section->device));
+        if ( section->ap != NULL ) {


sardemff7 · 2018-12-16T11:22:58Z

nm/src/nm.c

@@ -546,8 +551,11 @@ _j4status_nm_device_unmonitor(G_GNUC_UNUSED gpointer key, gpointer data, G_GNUC_
    case NM_DEVICE_TYPE_WIFI:
        g_signal_handler_disconnect(section->device, section->bitrate_handler);
        g_signal_handler_disconnect(section->device, section->active_access_point_handler);
-        if ( section->ap != NULL )
+        if ( section->ap != NULL ) {


wentasah added 5 commits December 16, 2018 09:40

nm: Use nm_utils_ssid_to_utf8()

0bdf161

The documentation [1] says that conversion of SSID to human readable string should be done only with this function. [1] https://developer.gnome.org/libnm-util/stable/libnm-util-nm-utils.html#nm-utils-ssid-to-utf8

nm: Suppress unused parameter warnings

852e6b0

Signed-off-by: Michal Sojka <michal.sojka@cvut.cz>

nm: Set data.ssid to NULL after unreferencing

17e5305

This ensures that when _j4status_nm_device_update() is called again but data.ssid is not set to a new value, we do not unreference possibly already freed variable.

sardemff7 self-assigned this Dec 16, 2018

sardemff7 requested changes Dec 16, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NM plugin fixes #13

NM plugin fixes #13

wentasah commented Dec 16, 2018

sardemff7 left a comment

sardemff7 Dec 16, 2018

sardemff7 Dec 16, 2018

sardemff7 Dec 16, 2018

sardemff7 Dec 16, 2018

sardemff7 Dec 16, 2018

sardemff7 Dec 16, 2018

sardemff7 Dec 16, 2018

sardemff7 Dec 16, 2018

sardemff7 Dec 16, 2018

sardemff7 Dec 16, 2018

NM plugin fixes #13

Are you sure you want to change the base?

NM plugin fixes #13

Conversation

wentasah commented Dec 16, 2018

sardemff7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment