I was thinking about creating a connectivity status library that would use NetworkManager by default and Netlink if it's not available. The latter part could be pretty much based on NetworkManager's NMPlatform while the first is simple dbus interfacing.

Yeah, GNetworkMonitor already uses netlink to check connectivity. It just doesn't try to deal with captive portal detection. And I think we're fine with requiring NM to be around to figure that out.

A NetworkManager GNetworkMonitor with this information would be great to have. It seems reasonable to expect that non-NM users need to implement a GNetworkMonitor for similar information.

ConnMan detects captive portals, but if I'm reading the source correctly, it seems to distinguish via the difference between "ready" and "online", both of which are treated as online states by <https://github.com/jukkar/connman-network-monitor/>. Unfortunately, according to the submitter of <http://cgit.freedesktop.org/telepathy/telepathy-mission-control/commit/?id=393d6c8b6e64c487c8a426443d3b285c35093dce>, "ready" can also mean "behind a proxy", which we want to treat as online (at least in applications with proxy support).

We don't generally want apps to think too hard about portals; they should mostly treat it the same as "not connected". There should be a single service (ie, the shell) that worries about informing the user about what's going on, rather than having every app simultaneously telling the user they need to log in.

We may want to expose it as a "partial connectivity" state though, especially since some portals will allow some traffic through, but block others.

(Currently GNetworkMonitor reports behind-a-portal as "connected", so we need GNetworkMonitor work either way.)

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

Created attachment 292077 [details] [review]
gio: add network connectivity state to GNetworkMonitor

Add a property to GNetworkMonitor indicating the level of network
connectivity: none/local, limited, stuck behind a portal, or full.

The default implementation just returns none or full depending on the
value of is-available.

Created attachment 292078 [details] [review]
gio: Add GNetworkMonitor impl based on NetworkManager

Which implements the new GNetworkConnectivity property.

Created attachment 292079 [details] [review]
gio: Add GNetworkMonitor impl based on NetworkManager

Which implements the new GNetworkConnectivity property.

Created attachment 292084 [details] [review]
gio: add network connectivity state to GNetworkMonitor

Add a property to GNetworkMonitor indicating the level of network
connectivity: none/local, limited, stuck behind a portal, or full.

The default implementation just returns none or full depending on the
value of is-available.

Created attachment 292085 [details] [review]
gio: Add GNetworkMonitor impl based on NetworkManager

Which implements the new GNetworkConnectivity property.

Created attachment 292089 [details] [review]
gio: Add GNetworkMonitor impl based on NetworkManager

Which implements the new GNetworkConnectivity property.

Last version removes our NM monitor's handling of the "network-changed" signal, and instead we become a subclass of the netlink monitor, init it, and let it handle that signal. We only take care of the 2 properties which we can handle properly.

Review of attachment 292084 [details] [review]:

::: gio/gnetworkmonitor.c
@@ +134,3 @@
+  g_object_get (G_OBJECT (monitor), "connectivity", &connectivity, NULL);
+
+  if (connectivity == (GNetworkConnectivity) -1)

This looks questionable, given that the property type is enum, and -1 is not one of the enum values.
Suggest to add G_NETWORK_CONNECTIVITY_UNKNOWN or make the property an it with suitable range.

::: gio/gnetworkmonitor.h
@@ +70,3 @@
+GNetworkMonitor      *g_network_monitor_get_default           (void);
+
+GLIB_AVAILABLE_IN_2_32

This should probably be a separate bugfix (GLIB_AVAILABLE_IN_ALL -> GLIB_AVAILABLE_IN_2_32)

(In reply to comment #13)
> Review of attachment 292084 [details] [review]:
> 
> ::: gio/gnetworkmonitor.c
> @@ +134,3 @@
> +  g_object_get (G_OBJECT (monitor), "connectivity", &connectivity, NULL);
> +
> +  if (connectivity == (GNetworkConnectivity) -1)
> 
> This looks questionable, given that the property type is enum, and -1 is not
> one of the enum values.
> Suggest to add G_NETWORK_CONNECTIVITY_UNKNOWN or make the property an it with
> suitable range.

This is trying to catch the fact that the monitor module we're using might not implement the connectivity property. I reworked this differently, using g_object_get_property().

> ::: gio/gnetworkmonitor.h
> @@ +70,3 @@
> +GNetworkMonitor      *g_network_monitor_get_default           (void);
> +
> +GLIB_AVAILABLE_IN_2_32
> 
> This should probably be a separate bugfix (GLIB_AVAILABLE_IN_ALL ->
> GLIB_AVAILABLE_IN_2_32)

Sure.

(In reply to comment #14)
> (In reply to comment #13)
> > Review of attachment 292084 [details] [review] [details]:
> > 
> > ::: gio/gnetworkmonitor.c
> > @@ +134,3 @@
> > +  g_object_get (G_OBJECT (monitor), "connectivity", &connectivity, NULL);
> > +
> > +  if (connectivity == (GNetworkConnectivity) -1)
> > 
> > This looks questionable, given that the property type is enum, and -1 is not
> > one of the enum values.
> > Suggest to add G_NETWORK_CONNECTIVITY_UNKNOWN or make the property an it with
> > suitable range.
> 
> This is trying to catch the fact that the monitor module we're using might not
> implement the connectivity property. I reworked this differently, using
> g_object_get_property().

Actually, it cannot be done like that. I tried using g_param_spec_get_redirect_target() but given that the base network monitor interface already implements this in exactly the same way in that patch, we would always have something implementing "connectivity" whatever the platform. So this fallback is just unnecessary.

Created attachment 292119 [details] [review]
gio: Correct the "available in" for GNetworkMonitor

They were marked as available in all versions when the main interface
was actually added in glib 2.32.

Created attachment 292120 [details] [review]
gio: add network connectivity state to GNetworkMonitor

Add a property to GNetworkMonitor indicating the level of network
connectivity: none/local, limited, stuck behind a portal, or full.

The default implementation just returns none or full depending on the
value of is-available.

Created attachment 292122 [details] [review]
gio: Add GNetworkMonitor impl based on NetworkManager

Which implements the new GNetworkConnectivity property.

Nowadays we detect captive portals with dnssec-trigger (daemon) in distributions that use it becase dnssec-trigger (panel) needs to ask the user whether he wants to enter hotspot signon mode. For dnssec-trigger, the hotspot signon mode is currently implemented as a permissive mode where DNSSEC is turned off.

If I understand correctly, when the integration between dnssec-trigger and NetworkManager reaches the level that NM will know the status od dnssec-trigger and will update its status accordingly, gnome network monitor will handle that, right?

(In reply to comment #19)
> Nowadays we detect captive portals with dnssec-trigger (daemon) in
> distributions that use it becase dnssec-trigger (panel) needs to ask the user
> whether he wants to enter hotspot signon mode. For dnssec-trigger, the hotspot
> signon mode is currently implemented as a permissive mode where DNSSEC is
> turned off.
> 
> If I understand correctly, when the integration between dnssec-trigger and
> NetworkManager reaches the level that NM will know the status od dnssec-trigger
> and will update its status accordingly, gnome network monitor will handle that,
> right?

The code above doesn't care one bit how it's implemented. This isn't a system API, it's an application API, and knowing how the portal was detected is therefore uninteresting. So the answer to your question is yes. It will handle it. And it's GNetworkMonitor, not GNOME's.

Review of attachment 292119 [details] [review]:

sure

Review of attachment 292120 [details] [review]:

Looks good to me.

Review of attachment 292122 [details] [review]:

is the idea that we use this implementation by default ? or only in applications that specifically care about portals ? if so, which ones are those ?

(In reply to comment #23)
> Review of attachment 292122 [details] [review]:
> 
> is the idea that we use this implementation by default ?

Yes.

> or only in
> applications that specifically care about portals ? if so, which ones are those
> ?

This is useful for applications to inform the user that they're not fully connected to the internet for example. I agree that the "portal" step isn't that useful for applications, except that it's a good idea for applications that use the network to get as much context as possible. If you have an application opened and you connect to the network, you'll see the status transitioning from "access to local network", "portal", full internet access". This means that you can get yourself ready in the "portal" case for when you get to "full internet access", for example.

It could be useful to show the connection level in gnome-control-center as well. It's not so useful in gnome-shell, obviously, as you need more plumbing to show the captive portal window, etc. so it's better to talk to NM directly.

Created attachment 292155 [details] [review]
gio: Add GNetworkMonitor impl based on NetworkManager

Which implements the new GNetworkConnectivity property.

Updated patch removes a stray debug print, and fixes the fact that NM doesn't follow standard D-Bus spec for properties.

Dan, one thing about the GNetworkConnectivity enum I didn't really like:
There's no difference between 1) not connected to any network, cable unplugged, wifi off and 2) cable plugged in to a local LAN with no way out to the internet, as far as GNetworkConnectivity is concerned.

Applications would need to combine network-available and connectivity to get that information, eg:
1) network_available == FALSE && connectivity == LOCAL
2) network_available == TRUE && connectivity == LOCAL

Should we add a new enum member that combines this information so applications can just monitor "connectivity"?

Comment on attachment 292155 [details] [review]
gio: Add GNetworkMonitor impl based on NetworkManager

>+G_DEFINE_TYPE_WITH_CODE (GNetworkMonitorNm, g_network_monitor_nm, G_TYPE_NETWORK_MONITOR_NETLINK,

The normal GLib/Gtk+ capitalization rule (retroactively defined so as to be compatible with the most existing symbols) is "two-letter acronyms have both letters capitalized [IO, UI, IP, etc], longer acronyms are initial caps only". So "GNetworkMonitorNM".

>+  v = g_dbus_proxy_get_cached_property (nm->priv->proxy, "PrimaryConnection");

Does it need to keep track of network-available itself? Can't it just let the base class do it?

Note that only a connection that has the default route can be PrimaryConnection, so if you have a connection that intentionally configures only link-local connectivity, and that's the only active connection, then there will be no PrimaryConnection.

>+  if (primary_connection == NULL ||
>+      *primary_connection == '\0' ||

object-path-valued properties can't be "". (Or is that just a dbus-glib-ism?) Either way, when there's no PrimaryConnection, NM will set the property to "/".

>+  dict = g_variant_dict_new (asv);
>+  if (!dict) {
>+    g_warning ("Failed to handle PropertiesChanged signal from NetworkManager");
>+    return;
>+  }

brace style

>+struct _GNetworkMonitorNmClass {
>+  GNetworkMonitorNetlinkClass parent_class;
>+
>+  /*< private >*/
>+  /* Padding for future expansion */
>+  gpointer padding[8];

You don't need that since it's not an exported type.

(In reply to comment #26)
> Updated patch removes a stray debug print, and fixes the fact that NM doesn't
> follow standard D-Bus spec for properties.

Yeah, that will be fixed once the daemon is ported to GDBus.

(In reply to comment #27)
> Dan, one thing about the GNetworkConnectivity enum I didn't really like:
> There's no difference between 1) not connected to any network, cable unplugged,
> wifi off and 2) cable plugged in to a local LAN with no way out to the
> internet, as far as GNetworkConnectivity is concerned.

I think the reason I didn't distinguish those is because in order to get it right, you have to be able to recognize that virbr0 isn't "a local LAN with no way out to the internet" (unless it has a physical device as one of its slaves...). Maybe this is something NM should be figuring out for you. (Though it's too late for that to happen in NM 1.0 at this point.)

Created attachment 292191 [details] [review]
gio: Add GNetworkMonitor impl based on NetworkManager

Which implements the new GNetworkConnectivity property.

(In reply to comment #28)
> (From update of attachment 292155 [details] [review])
> >+G_DEFINE_TYPE_WITH_CODE (GNetworkMonitorNm, g_network_monitor_nm, G_TYPE_NETWORK_MONITOR_NETLINK,
> 
> The normal GLib/Gtk+ capitalization rule (retroactively defined so as to be
> compatible with the most existing symbols) is "two-letter acronyms have both
> letters capitalized [IO, UI, IP, etc], longer acronyms are initial caps only".
> So "GNetworkMonitorNM".

Done.

> >+  v = g_dbus_proxy_get_cached_property (nm->priv->proxy, "PrimaryConnection");
> 
> Does it need to keep track of network-available itself? Can't it just let the
> base class do it?

I wanted to be sure that the status would be synchronised, and to help with comment 27.

> Note that only a connection that has the default route can be
> PrimaryConnection, so if you have a connection that intentionally configures
> only link-local connectivity, and that's the only active connection, then there
> will be no PrimaryConnection.
> 
> >+  if (primary_connection == NULL ||
> >+      *primary_connection == '\0' ||
> 
> object-path-valued properties can't be "". (Or is that just a dbus-glib-ism?)
> Either way, when there's no PrimaryConnection, NM will set the property to "/".

I've removed the PrimaryConnection handling, and I'm only using NM's connectivity property, which is more fine-grained than our version.

> >+  dict = g_variant_dict_new (asv);
> >+  if (!dict) {
> >+    g_warning ("Failed to handle PropertiesChanged signal from NetworkManager");
> >+    return;
> >+  }
> 
> brace style

Fixed.

> >+struct _GNetworkMonitorNmClass {
> >+  GNetworkMonitorNetlinkClass parent_class;
> >+
> >+  /*< private >*/
> >+  /* Padding for future expansion */
> >+  gpointer padding[8];
> 
> You don't need that since it's not an exported type.

Removed.

(In reply to comment #29)
> (In reply to comment #26)
> > Updated patch removes a stray debug print, and fixes the fact that NM doesn't
> > follow standard D-Bus spec for properties.
> 
> Yeah, that will be fixed once the daemon is ported to GDBus.

That's fine, I should have tested it, is all.

> (In reply to comment #27)
> > Dan, one thing about the GNetworkConnectivity enum I didn't really like:
> > There's no difference between 1) not connected to any network, cable unplugged,
> > wifi off and 2) cable plugged in to a local LAN with no way out to the
> > internet, as far as GNetworkConnectivity is concerned.
> 
> I think the reason I didn't distinguish those is because in order to get it
> right, you have to be able to recognize that virbr0 isn't "a local LAN with no
> way out to the internet" (unless it has a physical device as one of its
> slaves...). Maybe this is something NM should be figuring out for you. (Though
> it's too late for that to happen in NM 1.0 at this point.)

That's an edge case, probably. I think NM should be handling this, the question is whether we want to expand the enum right now, or try and do that later and have it be in the wrong order.

(In reply to comment #32)
> > I think the reason I didn't distinguish those is because in order to get it
> > right, you have to be able to recognize that virbr0 isn't "a local LAN with no
> > way out to the internet" (unless it has a physical device as one of its
> > slaves...). Maybe this is something NM should be figuring out for you. (Though
> > it's too late for that to happen in NM 1.0 at this point.)
> 
> That's an edge case, probably.

Well, it's not. virbr0 is always up on Fedora, so if you treat "virbr0 is up but doesn't have default route" the same as "eth0 is up but doesn't have default route", then you'll never report the "absolutely no connectivity" case.

> I think NM should be handling this, the question
> is whether we want to expand the enum right now, or try and do that later and
> have it be in the wrong order.

You could leave a hole in the numbering...

Created attachment 292201 [details] [review]
gio: add network connectivity state to GNetworkMonitor

Add a property to GNetworkMonitor indicating the level of network
connectivity: none/local, limited, stuck behind a portal, or full.

The default implementation just returns none or full depending on the
value of is-available.

Created attachment 292202 [details] [review]
gio: Add GNetworkMonitor impl based on NetworkManager

Which implements the new GNetworkConnectivity property.

Comment 30 wasn't actually an updated patch... I also added a hole in the enum numbering. Good to go?

yup

Attachment 292119 [details] pushed as ed68d80 - gio: Correct the "available in" for GNetworkMonitor
Attachment 292201 [details] pushed as 8d08b82 - gio: add network connectivity state to GNetworkMonitor
Attachment 292202 [details] pushed as 485a690 - gio: Add GNetworkMonitor impl based on NetworkManager

Hi,

I was wondering whether a header or a define was missed, as I am getting a compilation error on G_TYPE_NETWORK_CONNECTIVITY (undeclared indentifier), or did I miss something, after commit 8d08b82?

With blessings, thank you!

That would imply that gioenumtypes.[ch] need to be rebuilt, to pick up the new enum in gioenums.h. Maybe the dependencies are wrong in the win32 build so that doesn't happen automatically?

Hello Dan,

Yup, the Visual Studio build does not re-generate gioenumtypes.[ch] as that is normally distributed with the tarball.  Perhaps I should add a custom build step to do that?  So doing so would fix this-my fault, should have thought about this earlier:|

With blessings, thank you!