Created attachment 265140 [details] [review]
Windows implementation of gnetworkmonitor

As i needed the gnetworkmonitor on windows for my application i wrote such an implementation. I'm attaching this one with the request for a review. It makes use of a Windows function that is shipped with >= Windows 2000. Should be fine for us.

Comment on attachment 265140 [details] [review]
Windows implementation of gnetworkmonitor

Cool!

>+  ret = GetIpForwardTable (ip_forward_table, &size, 0);

This function only returns IPv4 data... it looks like you want GetIpForwardTable2, passing AF_UNSPEC to get both IPv4 and IPv6 routes, and then making minor adjustments to the rest of the code to deal with the difference in data structures. (Actually, this should simplify things, since the new structures include a prefix length directly rather than having a mask that you have to calculate the prefix length from.)

>+      g_warning("GetIpForwardTable () failed: %ld\n", ret);

need a space before the "(" here (and in a bunch of other places)

>+static void
>+win_network_monitor_process_table (GNetworkMonitorWindows *win,
>+                                   PMIB_IPFORWARDTABLE table,
>+                                   int init)

line up the arguments:

  (GNetworkMonitorWindows *win,
   PMIB_IPFORWARDTABLE     table,
   int                     init)

well, and "init" should be a gboolean anyway.

>+  else if (table->dwNumEntries < win->priv->table->dwNumEntries)
>+    {
>+      /* Traverse old list and find removed entries */

It would be a lot simpler to compare the dwForwardDests and dwForwardMasks directly rather than converting to GInetAddressMask first...

But actually, you don't really need to figure out the adds and deletes yourself; you can just convert the entire table to GInetAddressMasks and then call g_network_monitor_base_set_networks() on the whole list at once. (The reason the netlink-based version doesn't do it that way is because it gets individual add/remove notifications rather than getting a whole new routing table every time something changes.)

>+    /* Wait for network change event */
>+    if (WaitForSingleObject (win->priv->overlap.hEvent, INFINITE) ==

You don't need to create your own thread to do this; GMainContext on Windows lets you poll EVENTs. Just create a source something like:

  win->priv->source = g_timeout_source_new (0);
  g_source_set_ready_time (win->priv->source, -1); /* Never timeout */
  g_source_set_callback (win->priv->source, (GSourceFunc) routes_changed_cb, win);

  win->priv->pollfd.fd = win->priv->overlap.hEvent;
  win->priv->pollfd.events = G_IO_IN;
  g_source_add_poll (win->priv->source, &win->priv->pollfd);

  g_source_attach (win->priv->source, g_main_context_get_thread_default ());\

and then "routes_changed_cb" will get called every time the overlap's event triggers.

>+        free (win->priv->table);

g_free(), not free().

>+g_network_monitor_windows_finalize (GObject *object)

(You'll need to g_source_destroy() and g_source_unref() your source here.)

>+  /* Padding for future expansion */
>+  gpointer padding[8];

This type isn't visible in the public API, so it doesn't need padding.

>+win32_sources = $(win32_actual_sources) \
>+	 gnetworkmonitorwindows.c \
>+	 gnetworkmonitorwindows.h

I'm not sure what "win32_actual_sources" is all about, but it probably makes more sense to add the files there rather than here.

Thanks for your feedback, i really appreciate it! Just a quick note before i will fix all the issues:

According to MSDN GetIpForwardTable2 is available since:

Minimum supported client
Windows Vista [desktop apps only]
Minimum supported server
Windows Server 2008 [desktop apps only]

Do we really want to set it to this "high" limit? I know that XP is eol but should we do it now?

Oh. Blah. Currently we support XP+. So I guess stick with IPv4-only for now, but at least add a comment explaining this.

Also, since you're going to have to keep win_network_monitor_mask_get_len() now, I'll comment that it seems to be more complicated than it needs to be; you shouldn't need the "add" flag at all; the mask should always be a number whose binary representation would match "1*0*", so just read 1s from the top until you hit a 0, and then return.

Created attachment 265281 [details] [review]
Windows implementation of gnetworkmonitor v2

The second version fixes most of your reported issues, only the thread issue is left. At the moment i cannot get it to trigger the source event no matter which flags i set :(

Regarding win_network_monitor_mask_get_len() it is actually "0*1+0*" so i changed it a little bit.

Today I've tried to debug the source event issue using G_MAIN_POLL_DEBUG. Event gets fired but callback isn't triggered when using a timeout -1 (INFINITE) but it is called when using a timeout != -1.  Seems to be another bug...

Here is an extract of my log output. Note the WinNetMon Event which gets fired over and over

created context=003E7868
default context=003E7868
WinNetMon Event: 00000748
12:49:31: Available: 1
12:49:31: can reach: 1
polling context=003E7868 n=2 timeout=0
g_poll: waiting for 00000780 00000748
  WaitForMultipleObjectsEx(2, 0)
  wait returns 0
  got event 00000780
  WaitForMultipleObjectsEx(1, 0)
  wait returns 258 (WAIT_TIMEOUT)
g_main_poll(2) timeout: 0 - elapsed 0.0000000000 seconds [0x780 :i]
polling context=003E7868 n=2 timeout=-1
g_poll: waiting for 00000780 00000748
  WaitForMultipleObjectsEx(2, 0)
  wait returns 0
  got event 00000780
  WaitForMultipleObjectsEx(1, 0)
  wait returns 258 (WAIT_TIMEOUT)
g_main_poll(2) timeout: -1 - elapsed 0.0000000000 seconds [0x780 :i]
polling context=003E7868 n=2 timeout=-1
g_poll: waiting for 00000780 00000748
  WaitForMultipleObjectsEx(2, 0)
  wait returns 258 (WAIT_TIMEOUT)
  WaitForMultipleObjectsEx(2, -1)
  wait returns 1
  got event 00000748
g_main_poll(2) timeout: -1 - elapsed 12.9680000000 seconds [0x748 :i]
polling context=003E7868 n=2 timeout=-1
g_poll: waiting for 00000780 00000748
  WaitForMultipleObjectsEx(2, 0)
  wait returns 1
  got event 00000748
  WaitForMultipleObjectsEx(1, 0)
  wait returns 258 (WAIT_TIMEOUT)

* CONTINUES FOREVER *

Can we stick with the thread version in the meantime until this one is fixed? Today is the last day i've access to a Windows PC.

Created attachment 265303 [details] [review]
Windows implementation of gnetworkmonitor v3

This is v3 of my patch which does not use an own thread for signaling, instead it uses the windows function RegisterWaitForSingleObject (). 

I guess this is a solution we can live with. No unnecessary thread, instead it uses the windows wait thread and gsource is not needed at all.

(In reply to comment #7)
> Today I've tried to debug the source event issue using G_MAIN_POLL_DEBUG. Event
> gets fired but callback isn't triggered when using a timeout -1 (INFINITE) but
> it is called when using a timeout != -1.

Do you still have this version of the code? Can you attach it if so?

(In reply to comment #8)
> This is v3 of my patch which does not use an own thread for signaling, instead
> it uses the windows function RegisterWaitForSingleObject (). 

routes_changed_cb() is going to be called in a different thread, so you either need to add mutexes, or else have routes_changed_cb() just queue a (0-second) timeout source to run the real function in the correct thread.

* No, i do not have the patch of version 2 anymore. Sorry.

* I'm going with the 0-second timeout source.

Hi Dan, what about the review and integration of this patch?

(In reply to comment #11)
> Hi Dan, what about the review and integration of this patch?

I had pointed out a problem in comment 9 and you suggested you were going to fix it in comment 10, but you never attached an updated version

I'm sorry for the confusion Dan. You suggested changing the RegisterWaitForSingleObject() timeout value from INFINITE to 0 in order to receive the event only once. But this is already achieved with the dwFlags set to WT_EXECUTEONLYONCE.

The current implementation works like this: Wait until a change appear and then notify us *once* and after that re-register for the next event. There is no chance that routes_change_cb() is executed twice for one event.

With your idea we would end up polling the object over and over which would increase the cpu load.

The previous attached solution should be the best.

(In reply to comment #13)
> I'm sorry for the confusion Dan. You suggested changing the
> RegisterWaitForSingleObject() timeout value from INFINITE to 0 in order to
> receive the event only once.

Er? No. I was pointing out that the current version of your patch uses RegisterWaitForSingleObject() to set up its listener, but according to the docs for that (http://msdn.microsoft.com/en-us/library/windows/desktop/ms685061%28v=vs.85%29.aspx), the callback function you pass it will be called in a different thread. So you need to either (a) protect the data with a mutex, or (b) make routes_changed_cb() do nothing except queue a callback in the main thread.

Created attachment 287656 [details] [review]
Windows implementation of gnetworkmonitor v4

This version moves the processing into the original thread.

Comment on attachment 287656 [details] [review]
Windows implementation of gnetworkmonitor v4

There are still race conditions:

  - The monitor could be destroyed while routes_changed_cb() is running.

  - The monitor could be destroyed in between queueing the process_updates() timeout
    and when that timeout gets called

After thinking about this some, I don't think there's any way to make it work with having the wait operation hold a full ref on win. It might be possible with GWeakRef, although there are complications there as well (such as were to store the GWeakRef, and how it gets freed at the end).

I think it would probably be simpler to find the bug in the non-threaded version that kept it from working right.

Looks like the handle we receive is not a file handle and therefore the gsource approach does not work. Instead i tried to use a giochannel as a socket and i received notifications after a change has happend. Unfortunately the notification is coming over and over and there is no chance to read from the handle to stop it. Maybe we need to cancel the notification in this case and then re-request it. Haven't had a look at it yet.

@Dan: Do you know a better way to handle socket fds in this case?

Created attachment 291306 [details]
Standalone network monitor test application

I did some initial checks on the handle and got to know that it is neither a file descriptor nor a socket (see standalone application). I also tried to enable Dan's suggestion in the standalone application but it is not triggered at all.

So i started again with the sample application from Microsoft (http://msdn.microsoft.com/en-us/library/windows/desktop/aa366332%28v=vs.85%29.aspx) and discovered that it only works fine for ethernet and disabling/enabling the adapter but not for dropping/connecting wifi connections.

So what we currently have is:
 * a function NotifyRouteChange() that only works fine for ethernet but not for wifi (it is also only IPv4).
 * a gsource attempt that is not triggered yet.

Would it be possible to use the v4 version and add a g_idle_add in the callback function to change the routing table in the main context?

(In reply to comment #17)
> Looks like the handle we receive is not a file handle and therefore the gsource
> approach does not work.

It doesn't have to be a file handle. GSources on unix are based on file descriptors, but on Windows they're based on HANDLEs, and any handle that can be waited on ought to be usable in a GSource.

(In reply to comment #18)
> Would it be possible to use the v4 version and add a g_idle_add in the callback
> function to change the routing table in the main context?

No, that doesn't handle the race conditions mentioned in comment 16

Created attachment 365981 [details] [review]
ipv4/ipv6 network monitor for windows

This is an updated version of the above patches. It now make uses of the NotifyRouteChange2() function which includes ipv6 support.

Note: It offers a callback function only.

Review requested.

Review of attachment 365981 [details] [review]:

See the comments inline

::: gio/gnetworkmonitorwindows.c
@@ +129,3 @@
+
+      dest_addr = g_inet_address_new_from_bytes (dest, family);
+      if (!dest_addr) {

no braces

@@ +206,3 @@
+      case MibDeleteInstance:
+        if (!win_network_monitor_get_ip_info (route->DestinationPrefix, &family, &len, &dest))
+          {

no need for braces on single line blocks

@@ +258,3 @@
+
+  /* Cancel notification event */
+  CancelMibChangeNotify2 (&win->priv->handle);

you should check if the handle is not NULL before calling this

Review of attachment 365981 [details] [review]:

See the comments inline

::: gio/gnetworkmonitorwindows.c
@@ +129,3 @@
+
+      dest_addr = g_inet_address_new_from_bytes (dest, family);
+      if (!dest_addr) {

no braces

@@ +206,3 @@
+      case MibDeleteInstance:
+        if (!win_network_monitor_get_ip_info (route->DestinationPrefix, &family, &len, &dest))
+          {

no need for braces on single line blocks

@@ +258,3 @@
+
+  /* Cancel notification event */
+  CancelMibChangeNotify2 (&win->priv->handle);

you should check if the handle is not NULL before calling this

Created attachment 366005 [details] [review]
ipv4/ipv6 network monitor for windows - V2

Thanks for your review, I've updated the patch:

V2: Removed braces around single line blocks
    NULL check before cancel notification request

Review of attachment 366005 [details] [review]:

More nitpicks. Dan, the patch seems fine for me to go in. Please double check it since you made the previous reviews

::: gio/gnetworkmonitorwindows.c
@@ +123,3 @@
+
+      if (!win_network_monitor_get_ip_info (route->DestinationPrefix, &family, &len, &dest))
+          continue;

wrong indentation

@@ +202,3 @@
+      case MibDeleteInstance:
+        if (!win_network_monitor_get_ip_info (route->DestinationPrefix, &family, &len, &dest))
+            break;

wrong indentation

@@ +208,3 @@
+      case MibAddInstance:
+        if (!win_network_monitor_get_ip_info (route->DestinationPrefix, &family, &len, &dest))
+            break;

wrong indentation

@@ +234,3 @@
+    {
+       if (WSAGetLastError() != WSA_IO_PENDING)
+           g_warning("NotifyRouteChange2() error...%d\n", WSAGetLastError());

wrong indent, also no need for \n with g_warning

Created attachment 366006 [details] [review]
366005: ipv4/ipv6 network monitor for windows - V3

Thanks! Update to V3 of this patch:

V3: Fix wrong indentations
    Remove unnecessary line break in g_warning()

Review of attachment 366006 [details] [review]:

Dan doesn’t have much availability for patch review, so I’ll take this over from him.

::: gio/gnetworkmonitorwindows.c
@@ +70,3 @@
+win_network_monitor_get_ip_info(IP_ADDRESS_PREFIX  prefix,
+                                GSocketFamily     *family,
+                                gint              *len,

`gsize` rather than `gint`

@@ +71,3 @@
+                                GSocketFamily     *family,
+                                gint              *len,
+                                guint8           **dest)

Nitpick: Please put a space before the `(` in each function declaration/call.

`dest` should be a `const guint8 **`.

Also, conventionally, the length argument for a returned array comes after the array itself. i.e. `dest` should be before `len`.

@@ +76,3 @@
+    {
+      case AF_UNSPEC:
+        /* Fall-through: AF_UNSPEC deliveres both IPV4 and IPV6 infos, lets stick with IPV4 here */

Nitpick: s/lets/let’s/

@@ +99,3 @@
+  DWORD ret = 0;
+  GPtrArray *networks;
+  gint i, cnt = 0;

These should be `gsize` since they index into arrays. They certainly shouldn’t be signed.

@@ +106,3 @@
+  if (ret != ERROR_SUCCESS)
+    {
+      g_warning ("GetIpForwardTable2 () failed: %ld\n", ret);

g_warning() adds a \n to the end of the message itself, so you shouldn’t add one.

@@ +111,3 @@
+    }
+
+  networks = g_ptr_array_sized_new (routes->NumEntries);

This should be
```
networks = g_ptr_array_new_full (routes->NumEntries, g_object_unref);
```
otherwise all the GInetAddressMask instances will be leaked after the base_set_networks() call at the end of the function, since base_set_networks() takes its own ref on each of them (so we need to drop ours in this function).

@@ +116,3 @@
+      GInetAddress *dest_addr;
+      GInetAddressMask *network;
+      guint8 *dest;

`dest` should be `const`.

@@ +126,3 @@
+
+      dest_addr = g_inet_address_new_from_bytes (dest, family);
+      if (!dest_addr)

It’s not possible for g_inet_address_new_from_bytes() to fail, so this check is unnecessary.

@@ +131,3 @@
+      network = g_inet_address_mask_new (dest_addr, len, NULL);
+      g_object_unref (dest_addr);
+      g_return_val_if_fail (network != NULL, FALSE);

Are you sure the Windows API will always return `dest_addr` in the right form to produce a mask from? Would it be safer to have the following instead?
```
if (network == NULL)
  continue;
```

@@ +134,3 @@
+
+      g_ptr_array_add (networks, network);
+      cnt++;

There’s no need for `cnt` to be separate; you can just read `networks->len`.

@@ +137,3 @@
+    }
+
+  g_network_monitor_base_set_networks(G_NETWORK_MONITOR_BASE (win),

Nitpick: Add a space before the `(` in the function call.

@@ +148,3 @@
+             GSocketFamily           family,
+             gint                    dest_len,
+             guint8                 *dest)

`dest` should be a `const guint8 *`

Please order `dest_len` after `dest`, as per convention.

`dest_len` should be a `gsize`.

@@ +153,3 @@
+  GInetAddressMask *network;
+
+  if (dest)

`if (dest != NULL)` to make it more obvious that `dest` is a pointer, not a boolean.

@@ +160,3 @@
+  network = g_inet_address_mask_new (dest_addr, dest_len, NULL);
+  g_object_unref (dest_addr);
+  g_return_if_fail (network != NULL);

As above, are you sure this can never fail?

@@ +170,3 @@
+                GSocketFamily           family,
+                gint                    dest_len,
+                guint8                 *dest)

Same comments as above: make `dest` `const`, reorder the arguments so `dest` is before `dest_len`, and `dest_len` should be a `gsize`.

@@ +175,3 @@
+  GInetAddressMask *network;
+
+  if (dest)

`if (dest != NULL)`

@@ +182,3 @@
+  network = g_inet_address_mask_new (dest_addr, dest_len, NULL);
+  g_object_unref (dest_addr);
+  g_return_if_fail (network != NULL);

Same comment as above: are you sure this check can never fail?

This block of code (setting `dest_addr`, constructing `network`, and then checking it’s non-NULL) could be factored out into a helper function, since it’s the same in win_network_monitor_process_table(), add_network() and remove_network().

@@ +195,3 @@
+  GNetworkMonitorWindows *win = context;
+  GSocketFamily family;
+  guint8 *dest;

`const`

@@ +196,3 @@
+  GSocketFamily family;
+  guint8 *dest;
+  gint len;

`gsize`

@@ +226,3 @@
+  gboolean ret;
+
+  /* Read current ip routing table */

Nitpick: s/ip/IP/ in comments throughout

@@ +230,3 @@
+
+  /* Register for IPv4 and IPv6 route updates */
+  ret = NotifyRouteChange2 (AF_UNSPEC, (PIPFORWARD_CHANGE_CALLBACK) win_network_monitor_route_changed_cb, win, FALSE, &win->priv->handle);

What thread will win_network_monitor_route_changed_cb() be called in by Windows? I suspect that, just like the reviews on the previous versions of this patch said, you will need to marshal the data from that callback into GLib’s main thread in order to safely update the base network monitor without races. This basically involves:
 • Saving the thread-default main context on construction of the GNetworkMonitorWindows (g_main_context_get_thread_default()).
 • In the win_network_monitor_route_changed_cb() callback, calling g_main_context_invoke_full() on that saved GMainContext with a second callback function, and a closure containing the `route` and `type` information.
 • Into this new callback, move the code which is currently in win_network_monitor_route_changed_cb(), which calls remove_network() or add_network() as appropriate.

Let me know if you’ve got any questions about this.

@@ +234,3 @@
+    {
+       if (WSAGetLastError() != WSA_IO_PENDING)
+         g_warning("NotifyRouteChange2() error...%d", WSAGetLastError());

You should return the error as a `GError`. The GLib convention is that if a function returns a boolean and has a `GError**` parameter, it returns FALSE if and only if the error parameter is set.

You must also store the error here, and return it again on future calls to g_network_monitor_windows_initable_init(). See the documentation for g_initable_init().

Created attachment 366845 [details] [review]
ipv4/ipv6 network monitor for windows - V4

Thank you very much for your detailed feedback. I've updated my patch based on your review comments.

The following parts haven't been adjusted (yet):
 - To be on the safe side I've changed the implementation. In case a dest_addr is not "correct" we will skip it. Note: Windows API is not that detailed here.
 - Therefore we need the cnt variable
 - I'm not sure about: g_network_monitor_windows_initable_init(). I had a look at all the other sources and they do not implement a special handling here. Why is it necessary here? A new requirement?

PS: My patch is based on gnetworkmonitornetlink and so some of your findings also apply to this implementation.

Review of attachment 366845 [details] [review]:

(In reply to Jan-Michael Brummer from comment #27)
> The following parts haven't been adjusted (yet):
>  - To be on the safe side I've changed the implementation. In case a
> dest_addr is not "correct" we will skip it. Note: Windows API is not that
> detailed here.

Thanks.

>  - Therefore we need the cnt variable

I don’t think so. See my review comments below.

>  - I'm not sure about: g_network_monitor_windows_initable_init(). I had a
> look at all the other sources and they do not implement a special handling
> here. Why is it necessary here? A new requirement?

The other sources are likely wrong. The implementation advice for GInitable has changed a bit in the last few releases. All you need to do is store the GError and return if on a subsequent call to initable_init(). If the initial call to initable_init() succeeded, then you should return TRUE instead (so that means storing a GError* and a gboolean in the private data struct).

> PS: My patch is based on gnetworkmonitornetlink and so some of your findings
> also apply to this implementation.

Thanks, I’ll try and read through that code later and see if any improvements can be made.

::: gio/gnetworkmonitorwindows.c
@@ +98,3 @@
+static GInetAddressMask *
+get_network_mask(const guint8  *dest,
+                 guint          len,

`len` should be of type `gsize`.

@@ +99,3 @@
+get_network_mask(const guint8  *dest,
+                 guint          len,
+                 GSocketFamily  family)

Nitpick: Missing space before the `(`.

@@ +150,3 @@
+
+      g_ptr_array_add (networks, network);
+      cnt++;

You definitely can get rid of `cnt`, since `cnt` is incremented when, and only when, an entry is appended to `networks`. `networks->len` will always be exactly the same as `cnt`.

Note that `networks->len` is *not* the number of entries pre-allocated in the array (`routes->NumEntries` as passed in to the constructor). It’s the number of elements occupied in the array. You can verify this by printing `networks->len` after construction of `networks`. It will be 0.

@@ +164,3 @@
+             GSocketFamily           family,
+             const guint8           *dest,
+             gint                    dest_len)

`dest_len` should be of type `gsize`.

@@ +172,3 @@
+    g_network_monitor_base_add_network (G_NETWORK_MONITOR_BASE (win), network);
+    g_object_unref (network);
+  }

Nitpick: Coding style is wrong for this if-block. It should be:

if (…)
  {
    /* indented by 4 spaces */
  }

@@ +179,3 @@
+                GSocketFamily           family,
+                const guint8           *dest,
+                gint                    dest_len)

Same comments as for add_network() apply to this function.

@@ +229,3 @@
+win_network_monitor_route_changed_cb (PVOID                 context,
+                                     PMIB_IPFORWARD_ROW2   route,
+                                     MIB_NOTIFICATION_TYPE type)

Nitpick: Indentation of these arguments looks wrong.

@@ +234,3 @@
+  RouteData *route_data;
+
+  route_data = g_new (RouteData, 1);

Use g_new0() rather than g_new(), just to be sure that everything is initialised.

@@ +237,3 @@
+  route_data->route = route;
+  route_data->type = type;
+  route_data->win = win;

It’s possible for this closure to outlive the GNetworkMonitorWindows instance if, for example, g_main_context_invoke_full() gets called from one thread, then the final reference on the GNetworkMonitorWindows instance gets dropped from the main thread before the win_network_monitor_invoke_route_changed() callback is called in that thread.

The solution here is to actually use g_idle_source_new() and g_source_attach() manually (see how g_main_context_invoke_full() is implemented) to achieve the same effect — but also giving you the GSource pointer. You can then store that in GNetworkMonitorWindowsPrivate (alongside the main_context). In your finalize() function, if the stored GSource is non-NULL, call g_source_destroy() and g_source_unref() on it to cancel any pending callbacks. Apologies: my advice to use g_main_context_invoke_full() was slightly misleading.

@@ +254,3 @@
+  gboolean ret;
+
+  win->priv->main_context = g_main_context_get_thread_default ();

You should hold a reference to this GMainContext: use g_main_context_ref_thread_default() instead. Drop the reference in your finalize() function.

@@ +263,3 @@
+  if (ret != NO_ERROR)
+    {
+       if (WSAGetLastError() != WSA_IO_PENDING)

Nitpick: Missing space before `(` in the WSAGetLastError function call.

What happens if the error *is* WSA_IO_PENDING? Should the NotifyRouteChange2() call be retried? Or should you return a different error code? I don’t know what the semantics for WSA_IO_PENDING are, but currently in that case you are returning FALSE with no error set, which is not allowed.

Created attachment 366900 [details] [review]
ipv4/ipv6 network monitor for windows - V5

Again, thank you for your detailed feedback! I've updated the patch based on your review findings.

Is there a style checker which would fix the coding style issues automatically?

Review of attachment 366900 [details] [review]:

Getting there. Thanks for your updates to the patch.

The commit message for the patch needs a bit of rewriting too: generally we go for commit messages which explain the purpose of the patch, and any major things to be aware of. We don’t explain the revision history of developing the patch, since that information is useless when looking at the final git commit; it’s in Bugzilla instead. So the commit message here should be something along the lines of ‘added a Windows backend to GNetworkMonitor, using NotifyRouteChange2() (available on Vista and later). It marshals the route change callbacks to the thread-specific default main context the GNetworkMonitor was constructed in.’. Include any other information about the design of the patch which you think is relevant.

::: gio/gnetworkmonitorwindows.c
@@ +102,3 @@
+get_network_mask (const guint8  *dest,
+                  gsize          len,
+                  GSocketFamily  family)

Another nitpick: It makes a bit more sense to have the `family` argument before `dest` and `len`, since that’s the order they’re in in all the other functions.

@@ +130,3 @@
+  if (ret != ERROR_SUCCESS)
+    {
+      g_warning ("GetIpForwardTable2 () failed: %ld", ret);

I didn’t notice this before, but you should forward this as a GError to the caller, since win_network_monitor_process_table() is only called by initable_init(), which can forward the GError further. Typically, it’s much better to expose errors as GErrors rather than using g_warning(), since then the caller gets the choice of how to handle the error.

So, add a `GError **error` argument to this function, call g_set_error() here, and handle the error in the call site for win_network_monitor_process_table().

@@ +250,3 @@
+                         g_free);
+
+  g_source_attach (win->priv->route_change_source, win->priv->main_context);

This looks good, thanks. You’ve tested it and it works?

@@ +263,3 @@
+  if (win->priv->initialized)
+    {
+      if (win->priv->init_error)

Nitpick: `if (win->priv->init_error != NULL)`

@@ +265,3 @@
+      if (win->priv->init_error)
+        {
+          *error = g_error_copy (win->priv->init_error);

That won’t work, because error could be NULL (the caller doesn’t *have* to pass a GError* pointer in to the function). Use this instead:
```
g_propagate_error (error, g_error_copy (win->priv->init_error));
```

@@ +284,3 @@
+                    "NotifyRouteChange2() error...%d", ret);
+
+       win->priv->init_error = g_error_copy (*error);

Similarly, this won’t work because error might be NULL. Instead, do:
```
g_set_error (&win->priv->init_error, …);
g_propagate_error (error, g_error_copy (win->priv->init_error));
```

In order to avoid duplicating this g_propagate_error() code, you could restructure the function to be of the form:

```
static gboolean
g_network_monitor_windows_initable_init (…)
{
  if (!win->priv->initialized)
    {
      win->priv->main_context = g_main_context_ref_thread_default ();

      /* Read current IP routing table */
      …

      if (ret != NO_ERROR)
        g_set_error (&win->priv->init_error, …);

      win->priv->initialized = TRUE;
    }

  /* Forward the results. */
  if (win->priv->init_error != NULL)
    {
      g_propagate_error (error, g_error_copy (win->priv->init_error));
      return FALSE;
    }

  return TRUE;
}
```

@@ +305,3 @@
+
+  if (win->priv->init_error != NULL)
+    g_error_free (win->priv->init_error);

Nitpick: Instead of this if-block, you can just do:
```
g_clear_error (&win->priv->init_error);
```

(In reply to Jan-Michael Brummer from comment #29)
> Is there a style checker which would fix the coding style issues
> automatically?

Nope, although people generally use an editor which implements most of the styling for them automatically. Various files in GLib have relevant emacs/vim modelines, for example.

Created attachment 366923 [details] [review]
ipv4/ipv6 network monitor for windows - V6

Updated patch based on latest review including refactoring of g_network_monitor_windows_initable_init ()

As every patch before: patch has been tested successfully on a Windows system.

Review of attachment 366923 [details] [review]:

Great, thanks. I’ve spotted a couple of remaining minor problems, but I’ll fix those before I push it. Thanks a lot for your work on this.

::: gio/Makefile.am
@@ +347,3 @@
 	gwin32networking.h \
+	gnetworkmonitorwindows.c \
+	gnetworkmonitorwindows.h

Nitpick: Missing `\` from the second line here.

::: gio/gnetworkmonitorwindows.c
@@ +119,3 @@
+
+static gboolean
+win_network_monitor_process_table (GNetworkMonitorWindows *win, GError **error)

Nitpick: `error` should be on its own line.

::: gio/meson.build
@@ +395,3 @@
     'gwin32inputstream.h',
     'gwin32outputstream.h',
+    'gnetworkmonitorwindows.h',

Nitpick: This should actually be in the same list as gnetworkmonitorwindows.c, because we don’t want it to be a publicly installed header. The user should only ever see this object as a result of calling g_network_monitor_get_default().

Pushed to master with those nitpicks fixed.

(As commit f9aacf3952effff897ab42991b5ba9090de5d970.)

Thanks, i'll continue with the monitor for os x.

*** Bug 759989 has been marked as a duplicate of this bug. ***

Created attachment 366934 [details] [review]
gio: Rename gnetworkmonitorwindows to gwin32networkmonitor

This makes it more consistent with the other win32 objects in GIO. This
commit just renames the files; a follow-up commit will rename the
GObject.

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

Created attachment 366935 [details] [review]
gio: Rename GNetworkMonitorWindows to GWin32NetworkMonitor

This makes it more consistent with other GWin32* objects. No functional
changes.

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

Looking back at this, it needs to be a bit more consistent with the other gwin32*.[ch] files. Here are a couple of patches which rename it (but don’t change anything else). Can you review and test them please (I don’t have a Windows system to test on)?

Created attachment 366942 [details] [review]
Updated renaming patch

Updated version of your patch as you've missed one file and it wasn't compiling. Tested again under a Windows system: It's ok.

Sorry it took a little bit longer. I've tested the meson build for win32 and there's a bug in it.

Review of attachment 366942 [details] [review]:

Whoops, thanks for testing. What’s the issue with the meson build?

No problem :)

Meson finds stpcpy () and posix_memalign () during configuration but not during linking. Both shouldn't be defined as they are not available for windows.

Meson build problem is within mingw toolchain: Those functions are exported but not provides. Therfore meson is falling back to compiler built-in functions of stpcpy() and posix_memalign().

Solution:
 - Either set them in meson cross compile script to false:
    has_function_stpcpy = false
    has_function_posix_memalign = false

 - Change glib meson check to include the requested header file.

I modified my original patch to fix the remaining rename, thanks for your review. I didn't push your version, since I deliberately kept the renaming of the files and of the GObject separate to make things easier to look at in the logs.

Attachment 366934 [details] pushed as ba976f1 - gio: Rename gnetworkmonitorwindows to gwin32networkmonitor
Attachment 366935 [details] pushed as 3787e42 - gio: Rename GNetworkMonitorWindows to GWin32NetworkMonitor

(In reply to Jan-Michael Brummer from comment #44)
> Meson build problem is within mingw toolchain: Those functions are exported
> but not provides. Therfore meson is falling back to compiler built-in
> functions of stpcpy() and posix_memalign().
> 
> Solution:
>  - Either set them in meson cross compile script to false:
>     has_function_stpcpy = false
>     has_function_posix_memalign = false
> 
>  - Change glib meson check to include the requested header file.

Right, but those Meson problems are unrelated to the Windows network monitor.

Let’s discuss the Meson problems in bug #784995, which is already open about the Meson build on Windows.

This one can be closed again!