GNOME Bugzilla – Bug 792788
Network monitor support for macOS
Last modified: 2018-05-24 20:09:15 UTC
Created attachment 367229 [details] Network Monitor support for OSX I'm attaching a patch implementing network monitor support for OS X. This patch has been successfully tested on OS X High Sierra.
Created attachment 367231 [details] [review] Network Monitor support for OSX
Review of attachment 367231 [details] [review]: Thanks for the patch; it's a good first step, but it'll require some more iterations. ::: gio/gosxnetworkmonitor.c @@ +49,3 @@ +}; + +#define g_osx_network_monitor_get_type _g_osx_network_monitor_get_type If you don't use a leading `_` there's no need to do this @@ +94,3 @@ + +static gsize +get_bits(const guchar *p, gsize len) Coding style: - missing space between function and open parenthesis - missing newline between arguments @@ +103,3 @@ + break; + + if (i != len && p[i]) Coding style: please, add brackets around multiple one-line blocks. @@ +266,3 @@ + +static gpointer +osx_network_monitor_thread (gpointer user_data) This looks more like a job for a GSource, not for a thread constantly peeking at a socket file descriptor and sleeping if nothing happened. That's literally what the GLib main loop is for. @@ +282,3 @@ + + memset(msg, 0, len); + n = read(osx->priv->sockfd, msg, len); Coding style: missing space between function and open parenthesis. @@ +334,3 @@ + /* Note: We cannot use a GSocket here as we g_socket_new () for a PF_ROUTE is not supported + * and g_socket_new_from_fd () complains about an invalid argument. + * GSocket does not support sockets which do not offer a socket name. This seems to be worthy of investigation. @@ +347,3 @@ +g_osx_network_monitor_init (GOsXNetworkMonitor *osx) +{ + osx->priv = g_osx_network_monitor_get_instance_private (osx); There's really no reason to keep a pointer to the private data; get_instance_private() is going to be faster. @@ +366,3 @@ + if (read) + /* Start monitor thread */ + g_osx_network_monitor_start_thread (osx); You're starting a thread, but the init() may have its own cancellable. @@ +390,3 @@ + g_cancellable_cancel (osx->priv->cancellable); + if (osx->priv->thread != NULL) + g_thread_join (osx->priv->thread); I'm not really sure about this; you're cancelling the thread and joining it? @@ +393,3 @@ + + if (osx->priv->sockfd != 0) + close(osx->priv->sockfd); Coding style: missing space between function and open parenthesis. @@ +400,3 @@ + { + g_source_destroy (osx->priv->route_change_source); + g_source_unref (osx->priv->route_change_source); Why are you destroying the source and then unreffing it? ::: gio/gosxnetworkmonitor.h @@ +26,3 @@ +G_BEGIN_DECLS + +#define G_TYPE_OSX_NETWORK_MONITOR (_g_osx_network_monitor_get_type ()) No need to use a leading underscore: all symbols in GLib are not exported unless explicitly annotated. @@ +40,3 @@ + GNetworkMonitorBase parent_instance; + + GOsXNetworkMonitorPrivate *priv; No need to add this. ::: gio/meson.build @@ +359,3 @@ if glib_have_os_x_9_or_later unix_sources += files('gcocoanotificationbackend.c') + unix_sources += files('gwin32networkmonitor.c'); I'm pretty sure this is incorrect.
Review of attachment 367231 [details] [review]: ::: gio/gosxnetworkmonitor.c @@ +266,3 @@ + +static gpointer +osx_network_monitor_thread (gpointer user_data) Replacing it with a g_idle_xx() is ok? @@ +334,3 @@ + /* Note: We cannot use a GSocket here as we g_socket_new () for a PF_ROUTE is not supported + * and g_socket_new_from_fd () complains about an invalid argument. + * GSocket does not support sockets which do not offer a socket name. I guess the same applies to netlink network monitor and it luckily can fallback to g_source () @@ +366,3 @@ + if (read) + /* Start monitor thread */ + g_osx_network_monitor_start_thread (osx); Using this cancellable is ok? @@ +390,3 @@ + g_cancellable_cancel (osx->priv->cancellable); + if (osx->priv->thread != NULL) + g_thread_join (osx->priv->thread); I want to ensure that the task is done. @@ +400,3 @@ + { + g_source_destroy (osx->priv->route_change_source); + g_source_unref (osx->priv->route_change_source); C&P of my network monitor implementation for windows. Needs adjusting.
Created attachment 367246 [details] [review] Network Monitor support for OSX - V2 Updated patch: - Review comments adjusted - Switched thread to idle loop
Review of attachment 367246 [details] [review]: I see Emmanuele has already given a better code review than I could have... just one nit: ::: gio/Makefile.am @@ +301,3 @@ unix_sources += gcocoanotificationbackend.c endif +unix_sources += \ But in the meson.build, you added these files inside the OS X 9 condition. Here it's outside that condition. One or the other should change.
Emmanuele, could you please do another review round?
Review of attachment 367246 [details] [review]: Anyway, let's continue.... I'm going to assume the Mac networking code is correct, since I don't want to try to understand it. But I have some other comments. ::: gio/gosxnetworkmonitor.c @@ +63,3 @@ + ( (!(sa) || ((struct sockaddr *)(sa))->sa_len == 0) ? \ + sizeof(long) : \ + 1 + ( (((struct sockaddr *)(sa))->sa_len - 1) | (sizeof(long) - 1) ) ) This is quite obtuse; it demands a comment explaining how it works. You lost me at the bitwise or.... @@ +70,3 @@ +#define NEXT_SA(ap) ap = (struct sockaddr *) \ + ((caddr_t) ap + (ap->sa_len ? ROUNDUP(ap->sa_len, sizeof (u_long)) : \ + sizeof(u_long))) Also the code style is a bit inconsistent, there should be a space after ROUNDUP, and again after sizeof. You have sizeof right on the line above. Should be changed for SA_SIZE as well. @@ +140,3 @@ + family = G_SOCKET_FAMILY_IPV4; + dest = (guint8 *) &((struct sockaddr_in *)sa)->sin_addr; + len = get_bits(dest, 32); Please check all the function calls in this file to make sure they follow GNOME code style: get_bits (dest, 32) @@ +201,3 @@ + { + g_set_error (error, G_IO_ERROR, G_IO_ERROR_FAILED, + "Couldn't allocate required memory"); g_malloc will crash the application if it can't allocate memory, so no need to handle this case. The only time it can return NULL is if you pass zero. @@ +232,3 @@ + + g_network_monitor_base_set_networks (G_NETWORK_MONITOR_BASE (osx), + (GInetAddressMask **) networks->pdata, You're switching between two different styles of casting. Half the time, you use this style, but you also sometimes do e.g. (GInetAddressMask **)networks->pdata without the space there. Checking some other files, it seems like without the space is the more common code style for GLib, so I would follow that. @@ +300,3 @@ + * So we will have to use an own thread. + */ + osx->priv->idle = g_idle_add(osx_network_monitor_idle, osx); Missing space before opening parentheses. Also, the comment is stale and needs to be updated: you don't create your own thread anymore. (Which is good.) Now, switching to the main loop was a good change -- this is a lot closer than the previous patch -- but I'm worried about using the default main context here. The GNetworkMonitor could be created on a secondary thread, but its callbacks are going to be called on the main thread thread, and your code isn't prepared to handle that. You should use the thread-default main context instead. That's a bit more work, but not too bad: you'll have to use g_source_attach() here, keep a ref to the GSource in the priv struct, and destroy it with g_source_unref() in finalize. @@ +311,3 @@ + if (osx->priv->init_error != NULL) + { + g_propagate_error (error, g_error_copy (osx->priv->init_error)); I think you can avoid the g_error_copy if you then set osx->priv->init_error = NULL before returning, right?
Sorry, i missed your review. Unfortunately i cannot make use of this idle solution as those access are blocking only without an option to switch to polling :( Do you know another possible solution?
What exactly is blocking? You might need to use g_task_run_in_thread()...?
The read () operation is blocking, non blocking mode for this type of socket is not supported.
You need a secondary thread. I would use g_task_run_in_thread().
Adjusting component to gio in preparation for GitLab migration
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/1326.