Created attachment 369026 [details] [review]
gnetworkmonitornetlink: Fix potential GMainContext issue

Previously, the GSource would be attached to whatever GMainContext was
the thread default at the time; but that might no longer be the same as
the default at the time of constructing the GNetworkMonitor.

Save the default from construction time, so that source callbacks are
always invoked in the same GMainContext.

Signed-off-by: Philip Withnall <withnall@endlessm.com>

Created attachment 369027 [details] [review]
gnetworkmonitornetlink: Use a coarser-grained timer for dumps

By using g_timeout_source_new_seconds(), we can let timer wakeups be
coalesced by the scheduler, and reduce power consumption a bit. This
shouldn’t really affect the accuracy of the network monitoring.

Signed-off-by: Philip Withnall <withnall@endlessm.com>

Created attachment 369028 [details] [review]
gnetworkmonitornetlink: Refactor some code to reduce duplication

This should introduce no functional changes. Factor out some common
code, flip some arguments around to use the more conventional (data,
length) order, and move some memory management calls out of
if-blocks.

Signed-off-by: Philip Withnall <withnall@endlessm.com>

Created attachment 369029 [details] [review]
gnetworkmonitornetlink: Fix memory leaks on error paths

Use a common error handler to avoid leaks on the error paths.

Signed-off-by: Philip Withnall <withnall@endlessm.com>

Created attachment 369030 [details] [review]
gnetworkmonitornetlink: Fix a memory leak in unusual circumstances

If the GNetworkMonitorNetlink is finalised part-way through a dump
(after request_dump() is called, but before finish_dump() is called),
dump_networks was leaked. Fix that.

Signed-off-by: Philip Withnall <withnall@endlessm.com>

Created attachment 369031 [details] [review]
gnetworkmonitornm: Fix some minor GVariant memory leaks

Signed-off-by: Philip Withnall <withnall@endlessm.com>

Created attachment 369032 [details] [review]
gnetworkmonitornm: Use g_strv_contains() rather than reinventing it

This introduces no functional changes.

Signed-off-by: Philip Withnall <withnall@endlessm.com>

Review of attachment 369028 [details] [review]:

::: gio/gnetworkmonitornetlink.c
@@ +227,3 @@
   network = g_inet_address_mask_new (dest_addr, dest_len, NULL);
   g_object_unref (dest_addr);
+  g_return_val_if_fail (network != NULL, NULL);

Should this "return if fail" be moved to the consumers of the create_inet_address_mask? Otherwise it will yell here about "network" being NULL, but "add_network" and "remove_network" will happily run g_object_ref etc on it.

Review of attachment 369029 [details] [review]:

::: gio/gnetworkmonitornetlink.c
@@ +313,3 @@
     {
       g_warning ("Error on netlink socket: %s", error->message);
+      g_clear_error (&error);

I don't see the full context of this function, but maybe g_clear_error could also be called after "done:" label?

(In reply to Krzesimir Nowak from comment #8)
> Review of attachment 369028 [details] [review] [review]:
> 
> ::: gio/gnetworkmonitornetlink.c
> @@ +227,3 @@
>    network = g_inet_address_mask_new (dest_addr, dest_len, NULL);
>    g_object_unref (dest_addr);
> +  g_return_val_if_fail (network != NULL, NULL);
> 
> Should this "return if fail" be moved to the consumers of the
> create_inet_address_mask? Otherwise it will yell here about "network" being
> NULL, but "add_network" and "remove_network" will happily run g_object_ref
> etc on it.

Hmm, borderline, but I suppose so. I was basically thinking that all bets were off at that point, but we could try to be nice.

(In reply to Krzesimir Nowak from comment #9)
> Review of attachment 369029 [details] [review] [review]:
> 
> ::: gio/gnetworkmonitornetlink.c
> @@ +313,3 @@
>      {
>        g_warning ("Error on netlink socket: %s", error->message);
> +      g_clear_error (&error);
> 
> I don't see the full context of this function, but maybe g_clear_error could
> also be called after "done:" label?

I’d prefer to treat the scope of the GError allocations a little more locally.

(In reply to Krzesimir Nowak from comment #8)
> Review of attachment 369028 [details] [review] [review]:
> 
> ::: gio/gnetworkmonitornetlink.c
> @@ +227,3 @@
>    network = g_inet_address_mask_new (dest_addr, dest_len, NULL);
>    g_object_unref (dest_addr);
> +  g_return_val_if_fail (network != NULL, NULL);
> 
> Should this "return if fail" be moved to the consumers of the
> create_inet_address_mask? Otherwise it will yell here about "network" being
> NULL, but "add_network" and "remove_network" will happily run g_object_ref
> etc on it.

I’ve made that change locally. Doesn’t seem much point in updating the entire patch set on Bugzilla for it, but I can if people want.

Review of attachment 369029 [details] [review]:

I think the placement of `g_clear_error (&error)` is fine as it is here. If it were moved to the 'done:' block I'd suggest moving the three identical "error on netlink socket" errors there too, ie:

  if (len < 0)
    {
      retval = FALSE;
      goto done;
    }

  ...

  done:
    if (error != NULL)
      g_warning ("Error on netlink socket: %s", error->message);
    g_clear_error (&error);
    ...

Though this would split the error handling above and below a ~100 line loop. Up to you, Philip.

I agree with the suggestion to move the `g_return_if_fail` out to the consumers of `create_inet_address_mask`.

Those two comments aside, all patches LGTM.

(In reply to Will Thompson from comment #12)
> Though this would split the error handling above and below a ~100 line loop.
> Up to you, Philip.

Yeah, I prefer to keep the error handling localised and not split, so I won’t modify that patch further.

Thanks for the reviews, both of you. I’ll push these all shortly.

All pushed to master. I don't think any of them are particularly worth backporting to glib-2-54.

Attachment 369026 [details] pushed as 85f3bc1 - gnetworkmonitornetlink: Fix potential GMainContext issue
Attachment 369027 [details] pushed as 23fad11 - gnetworkmonitornetlink: Use a coarser-grained timer for dumps
Attachment 369028 [details] pushed as 88bf493 - gnetworkmonitornetlink: Refactor some code to reduce duplication
Attachment 369029 [details] pushed as 183c846 - gnetworkmonitornetlink: Fix memory leaks on error paths
Attachment 369030 [details] pushed as 61a8998 - gnetworkmonitornetlink: Fix a memory leak in unusual circumstances
Attachment 369031 [details] pushed as 35d4c7f - gnetworkmonitornm: Fix some minor GVariant memory leaks
Attachment 369032 [details] pushed as 8266238 - gnetworkmonitornm: Use g_strv_contains() rather than reinventing it