After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 750282 - Add g_network_monitor_get_network_metered() to get if the connection is metered
Add g_network_monitor_get_network_metered() to get if the connection is metered
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: network
unspecified
Other Linux
: Normal enhancement
: ---
Assigned To: gtkdev
gtkdev
Depends on: 741725
Blocks:
 
 
Reported: 2015-06-02 14:41 UTC by Richard Hughes
Modified: 2015-07-29 10:59 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
initial patch (7.60 KB, patch)
2015-06-02 14:42 UTC, Richard Hughes
reviewed Details | Review
gio: Add network metered information to GNetworkMonitor (10.54 KB, patch)
2015-07-21 15:55 UTC, Philip Withnall
committed Details | Review
gio: Add network metered information to GNetworkMonitor (11.82 KB, patch)
2015-07-21 16:41 UTC, Philip Withnall
rejected Details | Review

Description Richard Hughes 2015-06-02 14:41:38 UTC
In GNOME software we want to avoid downloading metadata on metered connections, such as GSM or tethered connections. This API allows us to get the metered status, which can be either auto-detected (and guessed) or set in the NM config files.
Comment 1 Richard Hughes 2015-06-02 14:42:50 UTC
Created attachment 304435 [details] [review]
initial patch
Comment 2 Thomas Haller 2015-06-02 14:54:52 UTC
Review of attachment 304435 [details] [review]:

::: gio/gnetworkmonitornm.c
@@ +143,3 @@
+        return FALSE;
+      default:
+        g_warning ("Unknown NM metered state %d", nm_metered);

Probably this "default" case never hits.

And if it hits, then it hits because NM has a bug at this place (unlikely) or it added a new enum value (more-likely).

In the latter case I think you don't want to warn because otherwise updating NM could result in such warnings. How about just making a best guess and silently "return TRUE"?
Comment 3 Richard Hughes 2015-06-02 16:22:27 UTC
I think the idea for the warning would be so that someone reports a bug and we add the new enum rather than guessing. I just copied the precedent for g_network_monitor_get_network_available() but I'm happy to change either default.
Comment 4 Emmanuele Bassi (:ebassi) 2015-06-02 16:28:40 UTC
The two usual behaviours for enumerations are:

 * g_assert_not_reached(); this way, if NM is updated to add a new enumeration value and GLib isn't updated to handle it, we'll get a SIGABRT.
 * g_warn_if_reached() + return default value; this way, if NM is updated to add a new enumeration value and GLib isn't updated to handle it, we'll get a critical warning, but the library will go on.

Both assertion and warning can be disabled at compile time, but we usually enforce crashing to avoid undefined state after a critical warning (even if developers keep trudging on and never fix critical warnings).

My personal preference is to use g_assert_not_reached(), but it's up to the GIO maintainers to choose.
Comment 5 Dan Winship 2015-06-12 14:15:24 UTC
Comment on attachment 304435 [details] [review]
initial patch

>+ * g_network_monitor_get_network_metered:
>+ * @monitor: the #GNetworkMonitor
>+ *
>+ * Checks if the network is metered. "Metered" here means that the
>+ * traffic flowing through the connection is subject to limitations,
>+ * for example set by service providers.

"Limitations" is too vague. You should give an example scenario of why you'd want to call this function.

>+   * If this information is not available then no networks willl be

typo "willl"

>+   * See also #GNetworkMonitor::network-available.

Only one colon. (properties have one colon, signals have two)

>+      case 0: /* unknown */
>+      case 1: /* yes */
>+      case 3: /* guess-yes */
>+        return TRUE;

Returning TRUE when NM says "unknown" seems like a bad idea, since in general machines running NM/GNOME are more likely to be on unmetered networks than metered ones. (And also, we default to FALSE in the unknown-because-you're-not-running-NM case, so we should return FALSE in the unknown-when-you-are-running-NM case too.)

[Actually, given that there are "guess-yes" and "guess-no" values, I don't think NM should ever return "unknown"; "unknown" requires the application to guess what it should do, but NM knows more about the network configuration and so can always make a better guess than the application can.]

>+      /* this is only available post 1.0 */

"post NM 1.0" (just to be completely clear)

>+  if (new_network_metered != nm->priv->network_metered)
>+    {
>+      nm->priv->network_metered = new_network_metered;
>+      g_object_notify (G_OBJECT (nm), "network-available");

cut+paste-o s/available/metered/

>+  g_object_class_override_property (gobject_class, PROP_NETWORK_METERED, "network-metered");

You need to override this in GNetworkMonitorBase too, or else gobject will spew warnings when you call g_network_monitor_get_network_metered() on a netlink or dummy GNetworkMonitor.
Comment 6 Dan Winship 2015-06-12 14:15:51 UTC
oh, i have no opinion on the default: case question
Comment 7 Matthias Clasen 2015-06-16 23:46:28 UTC
Review of attachment 304435 [details] [review]:

::: gio/gnetworkmonitor.c
@@ +368,3 @@
+   * system network management API should be used instead.
+   *
+   * If this information is not available then no networks willl be

typo: willl

@@ +371,3 @@
+   * marked as metered.
+   *
+   * See also #GNetworkMonitor::network-available.

Only one : for properties

::: gio/gnetworkmonitornm.c
@@ +203,3 @@
+    {
+      nm->priv->network_metered = new_network_metered;
+      g_object_notify (G_OBJECT (nm), "network-available");

This should notify network-metered, I guess
Comment 8 Philip Withnall 2015-07-21 15:55:40 UTC
Created attachment 307840 [details] [review]
gio: Add network metered information to GNetworkMonitor

Add a property to GNetworkMonitor indicating if the network
is metered, e.g. subject to limitations set by service providers, such
as billing per unit of traffic sent, or limiting the amount of traffic
per month).

The default value is FALSE.
Comment 9 Philip Withnall 2015-07-21 15:59:33 UTC
Here’s an updated patch which addresses the review comments so far, though the changes I’ve made to the GNetworkMonitor:network-metered documentation could do with some scrutiny.

One question, Richard: why did you go with a boolean for GNetworkMonitor:network-metered rather than exposing the full multi-state value from NetworkMonitor? I suspect that other backends could map whatever multi-state they use onto it, and it would give the application a bit more information to work with when choosing whether to devour bandwidth.

I’m tempted to expose the multi-state.
Comment 10 Philip Withnall 2015-07-21 16:41:59 UTC
Created attachment 307842 [details] [review]
gio: Add network metered information to GNetworkMonitor

Add a property to GNetworkMonitor indicating if the network
is metered, e.g. subject to limitations set by service providers, such
as billing per unit of traffic sent, or limiting the amount of traffic
per month).

The default value is G_NETWORK_METERED_UNKNOWN; other permissible values
follow the NetworkManager API, which seems general enough to be useful
to other monitor backends.

---

Here’s another version of the patch which exposes the multi-state from NetworkMonitor in the most obvious way possible. I think NM’s multi-state values should be generally useful for other monitor backends, and feel that exposing this extra few bits of information to apps could be useful. For example, you might want to query the user before starting a big file download if the metering status is guess-no (“it’s possible that this will cost you a bomb”) but not if the status is no.
Comment 11 Matthias Clasen 2015-07-26 18:39:15 UTC
Review of attachment 307840 [details] [review]:

This looks ok
Comment 12 Matthias Clasen 2015-07-26 18:40:52 UTC
Review of attachment 307842 [details] [review]:

Lets keep it simple and in line with network-available. If you want to get all the information, talk to NetworkManager directly.
Comment 13 Matthias Clasen 2015-07-27 10:48:07 UTC
Attachment 307840 [details] pushed as a80e7db - gio: Add network metered information to GNetworkMonitor
Comment 14 Philip Withnall 2015-07-29 10:59:51 UTC
(In reply to Matthias Clasen from comment #13)
> Attachment 307840 [details] pushed as a80e7db - gio: Add network metered
> information to GNetworkMonitor

Not quite sure what happened here but it looks like attachment #304435 [details] (Richard’s initial patch) got committed instead of attachment #307840 [details].

I have applied the inter-diff as commit 6b652b1a2e7c6f67e9576e4331da76971d54cc68.