GNOME Bugzilla – Bug 741725
[review bg/metered-connections-bgo741725] provide information about metered connections
Last modified: 2016-09-30 07:14:56 UTC
gnome-software currently applies heuristics to determine if connections are 'metered' to avoid downloading updates. It would be nice if we could push this information down to the NM level - that would allow us to let people override the heuristics with a checkbox in the network panel. From irc discussion: the grand plan was to have a) is this metered, b) what is your data cap, c) and have some byte counters that store used data, and d) some way to clear those counters on a specific date which sounds pretty awesome. For our purposes in gnome-software, just having part a) would already be a big step forward. Also see bug 740982 which might point at ways to improve the heuristics.
*** Bug 748032 has been marked as a duplicate of this bug. ***
First of all, thanks Thomas for finding the original bug for my duplicate. My bad for not searching thoroughly enough. I would be totally happy with addition of the "metered" API (use case "a") and an explicit setting in the WiFi settings page. Having a good default based on the #740982 would be awesome indeed, but could be a step 2. I'd rather have a simpler solution for this now than wait for a more complex solution later. In my own experience, the worst offender here is PackageKit. And unfortunately, with every 2 Fedora releases the automatic update mechanism changes slightly. First I needed to disable yum-updatesd plugin. Then disable PackageKit updates via gpk-prefs (this was when I reported https://bugzilla.redhat.com/show_bug.cgi?id=1081454 a year ago). Now that does not work anymore and it probably moved to dnf somehow. As I mentioned in the duplicate, dnf/PackageKit just downloaded 270 MB (!) of file lists (I have about 8 or so repositories enabled) over my mobile, and now I am on 6 kb/s for the next three weeks or I pay 5€ for 200 MB more.. which dnf would probably eat for dessert.
We should add a setting to NMSettingConnection whether a connection is metered. Possible values should be "unknown", "yes", "no". The NMDevice should expose a metered flag too. With possible values "unknown", "guess-yes", "guess-no", "yes", "no". "yes" and "now" would correspond to setting the metered values in NMSettingConnection. "unknown" would cause NM trying to guess it. For example, for Android, seems there is a DHCP option "ANDROID_METERED" ( http://www.lorier.net/docs/android-metered )
(In reply to Thomas Haller from comment #3) > For example, for Android, seems there is a DHCP option "ANDROID_METERED" ( > http://www.lorier.net/docs/android-metered ) The original link I found: http://stackoverflow.com/a/23879816/354393
Pushed initial implementation to bg/metered-connections-bgo741725 for review.
>> libnm-core: add 'metered' property to NMSettingConnection + if (priv->metered != NM_SETTING_CONNECTION_METERED_UNKNOWN && + priv->metered != NM_SETTING_CONNECTION_METERED_YES && + priv->metered != NM_SETTING_CONNECTION_METERED_NO) { + g_set_error_literal (error, + g_object_class_install_property + (object_class, PROP_METERED, + g_param_spec_enum (NM_SETTING_CONNECTION_METERED, "", "", + NM_TYPE_SETTING_CONNECTION_METERED_VALUE, When the property is of type g_param_spec_enum(), you cannot g_object_set() the value to an invalid enum value. So, you can drop the check in validate(). OTOH, it also means, that we cannot ever later extend this enum to have yet another value. I guess that is ok in this case, but something to be aware of(?) +/** + * NMSettingConnectionMeteredValue: the "Value" suffix seems different from our other enum names. Maybe just "NMSettingConnectionMetered"? +/** + * NMSettingConnectionMeteredValue: + * @NM_SETTING_CONNECTION_METERED_UNKNOWN: The connection metered property is unknown + * @NM_SETTING_CONNECTION_METERED_YES: The connection is metered + * @NM_SETTING_CONNECTION_METERED_NO: The connection is not metered + */ Since: 1.2? guint32 nm_setting_connection_get_gateway_ping_timeout (NMSettingConnection *setting); +NMSettingConnectionMeteredValue nm_setting_connection_get_metered (NMSettingConnection *setting); NM_AVAILABLE_IN_1_2 make[3]: Entering directory '/data/src/NetworkManager/docs/api' DOC Preparing build GEN settings-spec.xml DOC Scanning header files DOC Introspecting gobjects DOC Rebuilding template files DOC Building XML DOC Building HTML DOC Fixing cross-references xsltproc --xinclude --nonet --path "../../introspection:../../introspection" ../../tools/doc-generator.xsl ../../introspection/all.xml > spec.html ERR: Unable to find type 'NM_DEVICE_METERED_VALUE' Makefile:719: recipe for target 'spec.html' failed make[3]: *** [spec.html] Error 10 >> core: add 'metered' property to NMDevice + * NMDeviceMeteredValue: + * @NM_DEVICE_METERED_UNKNOWN: The device metered status is unknown + * @NM_DEVICE_METERED_YES: The device is metered and the value was statically set + * @NM_DEVICE_METERED_NO: The device is not metered and the value was statically set + * @NM_DEVICE_METERED_GUESS_YES: The device is metered and the value was guessed + * @NM_DEVICE_METERED_GUESS_NO: The device is metered and the value was guessed + **/ Since: 1.2? + g_object_class_install_property + (object_class, PROP_METERED, + g_param_spec_uint (NM_DEVICE_METERED, "", "", ah. uint? Good. #define NM_DEVICE_MTU "mtu" #define NM_DEVICE_HW_ADDRESS "hw-address" #define NM_DEVICE_TYPE_DESC "type-desc" /* Internal only */ #define NM_DEVICE_RFKILL_TYPE "rfkill-type" /* Internal only */ #define NM_DEVICE_IFINDEX "ifindex" /* Internal only */ #define NM_DEVICE_IS_MASTER "is-master" /* Internal only */ #define NM_DEVICE_MASTER "master" /* Internal only */ #define NM_DEVICE_HAS_PENDING_ACTION "has-pending-action" /* Internal only */ +#define NM_DEVICE_METERED "metered" maybe should move up, to the non-internal-only definitions. Same here: NMDeviceMeteredValue, I don't mind which/whether suffex, but we should find something consistent... + * + * Returns: the metered setting. + **/ +NMDeviceMeteredValue +nm_device_get_metered (NMDevice *device) Since: 1.2 and NM_AVAILABLE_1_2 in the header. >> core: add 'metered' flag to NMIP4Config +void +nm_ip4_config_set_metered (NMIP4Config *config, gboolean metered) +{ + NMIP4ConfigPrivate *priv = NM_IP4_CONFIG_GET_PRIVATE (config); + + priv->metered = !!metered; +} should raise a "notify:metered" signal... OTOH, you didn't add NM_IP4_CONFIG_METERED to introspection/nm-ip4-config.xml, hence it is not exported via D-Bus. We don't really need a gobject property for it... >> core: update device 'metered' property on connection state change if (value == G_MAXINT) { that causes a compile warning (pushed a fixup for it).
>> dhcp: update NMIP4Config metered flag according to option 43 maybe improve the subject line? It wasn't clear to me what "option 43" is... dhcp: detect NMIP4Config metered flag base on to DHCP option 43 (ANDROID_METERED)
New entries in libnm.ver must go to the libnm_1_2_0 section.
(In reply to Thomas Haller from comment #6) > >> libnm-core: add 'metered' property to NMSettingConnection > When the property is of type g_param_spec_enum(), you cannot g_object_set() > the value to an invalid enum value. So, you can drop the check in validate(). > > OTOH, it also means, that we cannot ever later extend this enum to have yet > another value. I guess that is ok in this case, but something to be aware > of(?) Would it be better to use a uint as done for the metered property of NMDevice? > the "Value" suffix seems different from our other enum names. Maybe just > "NMSettingConnectionMetered"? Ok. > xsltproc --xinclude --nonet --path "../../introspection:../../introspection" > ../../tools/doc-generator.xsl ../../introspection/all.xml > spec.html > ERR: Unable to find type 'NM_DEVICE_METERED_VALUE' > Makefile:719: recipe for target 'spec.html' failed > make[3]: *** [spec.html] Error 10 Yeah, the type declaration in the XML file was missing. > > Same here: NMDeviceMeteredValue, I don't mind which/whether suffex, but we > should find something consistent... Renamed to NMDeviceMetered. > >> core: add 'metered' flag to NMIP4Config > should raise a "notify:metered" signal... > > OTOH, you didn't add NM_IP4_CONFIG_METERED to > introspection/nm-ip4-config.xml, hence it is not exported via D-Bus. We > don't really need a gobject property for it... I dropped the property altogether. > > >> core: update device 'metered' property on connection state change > > if (value == G_MAXINT) { > > that causes a compile warning (pushed a fixup for it). Thanks, branch repushed with fixes for also other comments.
(In reply to Beniamino Galvani from comment #9) > (In reply to Thomas Haller from comment #6) > > >> libnm-core: add 'metered' property to NMSettingConnection > > When the property is of type g_param_spec_enum(), you cannot g_object_set() > > the value to an invalid enum value. So, you can drop the check in validate(). > > > > OTOH, it also means, that we cannot ever later extend this enum to have yet > > another value. I guess that is ok in this case, but something to be aware > > of(?) > > Would it be better to use a uint as done for the metered property of > NMDevice? Yeah, probably. We should keep libnm/libnm-glib and other functions using the enum type in arguments, but the NMDevice and NMSettingConnection properties should be 'uint' to allow adding future values. However, note that enums may actually be signed values depending on the compiler implementation. But it's OK to use them as unsigned as long as we don't have any values above MAXINT. > libnm-core: add 'metered' property to NMSettingConnection > core: add 'metered' property to NMDevice Just a thought, would it be simpler to just have an NMMetered enum that had unknown, yes, know, guess-yes, guess-no, and use it for both the NMSettingConnection value and the NMDevice value? The 'guess' values would be invalid for use with NMSettingConnection. Might be simpler for clients since they could use the same enum when constructing configs as well as when interpreting the device state. > dhcp: detect NMIP4Config metered flag based on ANDROID_METERED DHCP option Does this bit only work for dhclient? I think we should also try to add support for the systemd internal DHCP client here too. Rest looks OK to me.
(In reply to Dan Williams from comment #10) > (In reply to Beniamino Galvani from comment #9) > > Would it be better to use a uint as done for the metered property of > > NMDevice? > > Yeah, probably. We should keep libnm/libnm-glib and other functions using > the enum type in arguments, but the NMDevice and NMSettingConnection > properties should be 'uint' to allow adding future values. Okay. > Just a thought, would it be simpler to just have an NMMetered enum that had > unknown, yes, know, guess-yes, guess-no, and use it for both the > NMSettingConnection value and the NMDevice value? The 'guess' values would > be invalid for use with NMSettingConnection. Might be simpler for clients > since they could use the same enum when constructing configs as well as when > interpreting the device state. Indeed, and it's simpler also for developers :) > > dhcp: detect NMIP4Config metered flag based on ANDROID_METERED DHCP option > > Does this bit only work for dhclient? I think we should also try to add > support for the systemd internal DHCP client here too. The internal DHCP client doesn't support option 43 ("Vendor Specific Information") which is used to deliver the information about a metered connection. As far as I understand we never patch imported systemd files (apart from trivial integration fixes) and so the feature needs to be added upstream. Any suggestions?
> > > dhcp: detect NMIP4Config metered flag based on ANDROID_METERED DHCP option > > > > Does this bit only work for dhclient? I think we should also try to add > > support for the systemd internal DHCP client here too. > > The internal DHCP client doesn't support option 43 ("Vendor Specific > Information") which is used to deliver the information about a metered > connection. As far as I understand we never patch imported systemd files > (apart from trivial integration fixes) and so the feature needs to be added > upstream. Any suggestions? well, we can patch first our version and then send patches upstream. We should just try to deviate from original source as little as possible
nmc_property_connection_get_metered() should use localized names. But see also "cli: refactor property to string conversion" from th/default-dns-options-bgo749648. + g_set_error (error, 1, 0, _("'%s' is not valid; use [yes, no, unknown]"), val); should be + g_set_error (error, 1, 0, _("'%s' is not valid; use %s"), val, "[yes, no, unknown]"); so that the values don't get localized. verify() in NMSettingConnection checks the metered enum values. OTOH, NM_SETTING_CONNECTION_METERED is an uint, to be extensible. Either you make it extensible (no verify), or you leave it unrestricted for future values. OTOH, you can check: verify(): + if ( priv->metered == NM_METERED_GUESS_NO + || priv->metered == NM_METERED_GUESS_YES) { /* these values really make no sense ever on a connection and will always wrong. */ in libnm/nm-device.c: + g_object_class_install_property + (object_class, PROP_METERED, + g_param_spec_enum (NM_DEVICE_METERED, "", "", interestingly, you made NMDevice's propery an enum. I think you should define which properties are strictly limited to currently known values, and which not. NMConnectionSetting:metered? NMDevice:metered? Maybe let's make NMSettingConnection:metered strict, and NMDevice:metered open? It seems more likely that we add a valid enum value for NMDevice (like NM_METERED_GUESS_PRETTY_SURE). - metered = !!memmem (data, num, "ANDROID_METERED", STRLEN ("ANDROID_METERED")); + metered = !!memmem (data, num, "ANDROID_METERED", STRLEN ("ANDROID_METERED") + 1); »···if (num > 0) { »···»···gboolean metered; »···»···metered = !!memmem (data, num, "ANDROID_METERED", STRLEN ("ANDROID_METERED")); »···»···nm_ip4_config_set_metered (ip4_config, metered); »···} it doesn't actually matter, but could you call nm_ip4_config_set_metered() also in the else case? »···gboolean metered = FALSE; »···if (num > 0) »···»···metered = !!memmem (data, num, "ANDROID_METERED", STRLEN ("ANDROID_METERED")); »···nm_ip4_config_set_metered (ip4_config, metered); + case DHCP_OPTION_VENDOR_SPECIFIC: + r = lease_parse_binary(option, len, &lease->vendor_specific); + if ( indention
(In reply to Dan Williams from comment #10) > Yeah, probably. We should keep libnm/libnm-glib and other functions using > the enum type in arguments, but the NMDevice and NMSettingConnection > properties should be 'uint' to allow adding future values. (In reply to Thomas Haller from comment #13) > > I think you should define which properties are strictly limited to currently > known values, and which not. NMConnectionSetting:metered? NMDevice:metered? > > Maybe let's make NMSettingConnection:metered strict, and NMDevice:metered > open? > It seems more likely that we add a valid enum value for NMDevice (like > NM_METERED_GUESS_PRETTY_SURE). The metered property is now in available in 3 different objects: NMDevice(core), NMDevice(libnm), NMSettingConnection(libnm-core). This is what I was thinking of, but since I'm not very familiar with the overall architecture, please correct me if I'm wrong: - NMDevice(libnm) is used by clients and is a proxy for the core equivalent. Its metered property is readonly and we must be prepared to deal with any value read from the core; thus the property must be a uint. - The metered property in NMDevice(core) is never set externally but is changed only according some internal logic (copied from connection or guessed). For this reason I think that it can be defined as an enum; I guess it's ok if the type is different from the one used in libnm. - NMSettingConnection(libnm-core) is visible to clients and therefore should be a uint to allow new values to be used.
(In reply to Thomas Haller from comment #13) > nmc_property_connection_get_metered() > > should use localized names. But see also "cli: refactor property to string > conversion" from th/default-dns-options-bgo749648. Ok. > - metered = !!memmem (data, num, "ANDROID_METERED", STRLEN > ("ANDROID_METERED")); > + metered = !!memmem (data, num, "ANDROID_METERED", STRLEN > ("ANDROID_METERED") + 1); The result of sd_dhcp_lease_get_vendor_specific() is the option buffer as copied from the packet, which doesn't have a terminating zero: $ tcpdump -r android_metered.pcapng -Xv [...] Vendor-Option 43, length 15: 65.78.68.82.79.73.68.95.77.69.84.69.82.69.68 [...] 0x0140: 0274 702b 0f41 4e44 524f 4944 5f4d 4554 .tp+.ANDROID_MET 0x0150: 4552 4544 ff ERED. So I think it's ok without the +1. > »···if (num > 0) { > »···»···gboolean metered; > > »···»···metered = !!memmem (data, num, "ANDROID_METERED", STRLEN > ("ANDROID_METERED")); > »···»···nm_ip4_config_set_metered (ip4_config, metered); > »···} > > it doesn't actually matter, but could you call nm_ip4_config_set_metered() > also in the else case? Ok.
(In reply to Beniamino Galvani from comment #14) > (In reply to Dan Williams from comment #10) > > Yeah, probably. We should keep libnm/libnm-glib and other functions using > > the enum type in arguments, but the NMDevice and NMSettingConnection > > properties should be 'uint' to allow adding future values. > > (In reply to Thomas Haller from comment #13) > > > > I think you should define which properties are strictly limited to currently > > known values, and which not. NMConnectionSetting:metered? NMDevice:metered? > > > > Maybe let's make NMSettingConnection:metered strict, and NMDevice:metered > > open? > > It seems more likely that we add a valid enum value for NMDevice (like > > NM_METERED_GUESS_PRETTY_SURE). > > The metered property is now in available in 3 different objects: > NMDevice(core), NMDevice(libnm), NMSettingConnection(libnm-core). This > is what I was thinking of, but since I'm not very familiar with the > overall architecture, please correct me if I'm wrong: > > - NMDevice(libnm) is used by clients and is a proxy for the core > equivalent. Its metered property is readonly and we must be > prepared to deal with any value read from the core; thus the > property must be a uint. > > - The metered property in NMDevice(core) is never set externally but > is changed only according some internal logic (copied from > connection or guessed). For this reason I think that it can be > defined as an enum; I guess it's ok if the type is different from > the one used in libnm. > > - NMSettingConnection(libnm-core) is visible to clients and therefore > should be a uint to allow new values to be used. The actual question is the promises on the D-Bus API. On D-Bus API you have NMDevice:metered and NMSettingConnection:metered. What is the valid range for these values? Is it fixed (known already know) or will it be extended in the future? And: you can choose to say: NMSettingConnection:metered on D-Bus will never be extended beyong "unknown|yes|no", but at the same time expect that NMDevice:metered to be extended. There is no contradiction there as they are different values... one can be enum, the latter uint. On server side it doesn't matter too much what NMDevicePriv:metered is (as long as PROP_METERED will not violate the promises on the API). In principle you could do: get_property() ... case PROP_METERED: if (priv->metered < UNKNOWN || priv->metered > MAX) g_value_set_enum (value, UNKNOWN); else g_value_set_enum (value, priv->metered); break; (if that is useful for nm-core implementation). On client side (libnm) it doesn't make much sense to deviate from what the D-Bus API specifies. If on D-Bus the value will never be extended beyond known values, implementing the property as uint adds complexity and uncertainty without use. OTOH, if the D-Bus value range will be extended in the future, old clients will have problems to properly handle such connections. So, if you don't make libnm anticipate future changes now, we cannot do the changes later. Lastly, the combination of having NMSettingConnection:verify() rejecting values, while having PROP_METERED as guint doesn't make much sense. Either have it uint and do no validation at all. Or have the property of enum type and optionally enforce additional constraints (unknown|yes|no) via verify() -- if the latter constraints cannot be expressed by g_param_spec_enum() alone.
(In reply to Thomas Haller from comment #16) > The actual question is the promises on the D-Bus API. On D-Bus API you have > NMDevice:metered and NMSettingConnection:metered. > > What is the valid range for these values? Is it fixed (known already know) > or will it be extended in the future? > > And: you can choose to say: NMSettingConnection:metered on D-Bus will never > be extended beyong "unknown|yes|no", but at the same time expect that > NMDevice:metered to be extended. There is no contradiction there as they are > different values... one can be enum, the latter uint. Now I understand and this approach makes sense to me. > Lastly, the combination of having NMSettingConnection:verify() rejecting > values, while having PROP_METERED as guint doesn't make much sense. > Either have it uint and do no validation at all. Or have the property of > enum type and optionally enforce additional constraints (unknown|yes|no) via > verify() -- if the latter constraints cannot be expressed by > g_param_spec_enum() alone. Yeah, the property will be represented as an enum but additional checks will be needed to limit the subset of allowed values.
Pushed some fixups to branch bg/metered-connections-bgo741725.
in src/dev/nm-device.c, NM_DEVICE_METERED is READABLE (only), so it should not have a switch-case in set_property(). otherwise LGTM
+ if (priv->metered != NM_METERED_UNKNOWN && + priv->metered != NM_METERED_YES && + priv->metered != NM_METERED_NO) { + g_set_error (error, + NM_CONNECTION_ERROR, ^^^^^ indention. - g_message (" mtrd: %d", nm_ip4_config_get_metered (config)); + g_message (" mtrd: %d", (int) nm_ip4_config_get_metered (config)); otherwise really LGTM :)
> core: add 'metered' property to NMDevice > cli: add 'metered' property to device > libnm-core: add 'metered' property to NMSettingConnection These three commits are mixed, you should move the files to the appropriate commits. > core: add 'metered' property to NMDevice -g_return_val_if_fail (self != NULL, NM_METERED_UNKNOWN); +g_return_val_if_fail (NM_IS_DEVICE (self), NM_METERED_UNKNOWN); It is better to use NM_IS_DEVICE (self) because it can caught non-NULL but invalid objects. > libnm/nm-device.c /** * NMDevice:metered: * * Since: 1.2 **/ The property is not described. I think we should add support to ifcg-rh plugin (CONNECTION_METERED?) You might squash the fixup commits as they are easy fixes and make the branch more straightforward.
(In reply to Jiri Klimes from comment #21) > These three commits are mixed, you should move the files to the appropriate > commits. Ok. > > > core: add 'metered' property to NMDevice > -g_return_val_if_fail (self != NULL, NM_METERED_UNKNOWN); > +g_return_val_if_fail (NM_IS_DEVICE (self), NM_METERED_UNKNOWN); > It is better to use NM_IS_DEVICE (self) because it can caught non-NULL but > invalid objects. Fixed. > > I think we should add support to ifcg-rh plugin (CONNECTION_METERED?) Right, I've added it now. > > You might squash the fixup commits as they are easy fixes and make the > branch more straightforward. Squashed fixups, rebased on master and repushed. Thanks!
>> cli: add support for 'metered' connection property + else { + g_set_error (error, 1, 0, _("'%s' is not valid; use %s"), val, "[yes, no, unknown]"); + return FALSE; + } hm... or I don't know. For localized applications, do we actually want to parse "si, no, sconosciuto"? Or do we want to parse instead numeric values?... anyway, this should be consolidated after th/default-dns-options-bgo749648 merges -- but that doesn't block this branch. + nm_ip4_config_set_metered (dst, nm_ip4_config_get_metered (dst) | + nm_ip4_config_get_metered (src)); should be "||" >> core: update device 'metered' property on connection state change don't we have to update the metered flag on a new DHCP event? That is all.
(In reply to Thomas Haller from comment #23) > >> cli: add support for 'metered' connection property > > + else { > + g_set_error (error, 1, 0, _("'%s' is not valid; use %s"), val, > "[yes, no, unknown]"); > + return FALSE; > + } > > > hm... or I don't know. For localized applications, do we actually want to > parse "si, no, sconosciuto"? Or do we want to parse instead numeric > values?... > > anyway, this should be consolidated after th/default-dns-options-bgo749648 > merges -- but that doesn't block this branch. > You might add a function nmc_string_to_trivalent() similar to nmc_string_to_bool().
(In reply to Thomas Haller from comment #23) > >> cli: add support for 'metered' connection property > > + else { > + g_set_error (error, 1, 0, _("'%s' is not valid; use %s"), val, > "[yes, no, unknown]"); > + return FALSE; > + } > > > hm... or I don't know. For localized applications, do we actually want to > parse "si, no, sconosciuto"? Or do we want to parse instead numeric > values?... Other properties use non-localized values (see for example properties using nmc_property_set_bool()), so I think we should do the same here. > + nm_ip4_config_set_metered (dst, nm_ip4_config_get_metered (dst) | > + nm_ip4_config_get_metered (src)); > should be "||" Ok. > >> core: update device 'metered' property on connection state change > > don't we have to update the metered flag on a new DHCP event? Yes, now the flag is updated also after a renewal. (In reply to Jiri Klimes from comment #24) > You might add a function nmc_string_to_trivalent() similar to > nmc_string_to_bool(). Done, thanks.
pushed one fixup as suggestions. Feel free to reject it.
Would it be possible to serve the ANDROID_METERED option for ipv4.method=shared connection? Maybe default to metered=1 by default unless the user overrides it with metered=0 or decide by whether the default route goes via a metered connection. > nmcli: fix error message localization > + _("'%s' is ambiguous (%s)"), str, "on x off"); I think it might be a good idea to a /* Translators: .... */ comment above that.
(In reply to Lubomir Rintel from comment #27) > Would it be possible to serve the ANDROID_METERED option for > ipv4.method=shared connection? Maybe default to metered=1 by default unless > the user overrides it with metered=0 or decide by whether the default route > goes via a metered connection. Good idea. Inside NMSettingConnection:metered, the default would still be "unknown" (meaning: guess). Just for a shared connections we would employ completely different heuristics. I think that doesn't block this branch. Can be done in a follow up.
(In reply to Thomas Haller from comment #26) > pushed one fixup as suggestions. Feel free to reject it. Looks good, thanks. (In reply to Lubomir Rintel from comment #27) > Would it be possible to serve the ANDROID_METERED option for > ipv4.method=shared connection? Maybe default to metered=1 by default unless > the user overrides it with metered=0 or decide by whether the default route > goes via a metered connection. Yeah, I'll add this feature once the branch has been merged. > > nmcli: fix error message localization > > + _("'%s' is ambiguous (%s)"), str, "on x off"); > > I think it might be a good idea to a /* Translators: .... */ comment above > that. Comment added. After discussing on IRC, I pushed an additional commit that introduces a global PrimaryConnectionMetered property which makes the work easier for clients. They don't have anymore to get the primary connection, find the associated device and then query its metered status but can simply read the global property instead.
(In reply to Beniamino Galvani from comment #29) > (In reply to Thomas Haller from comment #26) > After discussing on IRC, I pushed an additional commit that introduces a > global PrimaryConnectionMetered property which makes the work easier for > clients. They don't have anymore to get the primary connection, find the > associated device and then query its metered status but can simply read the > global property instead. somehow NMManager must rise a nofify:p-c-metered signal when either the primary-connection changes or it's metered value.
(In reply to Thomas Haller from comment #30) > somehow NMManager must rise a nofify:p-c-metered signal when either the > primary-connection changes or it's metered value. Pushed some fixups.
(In reply to Beniamino Galvani from comment #31) > (In reply to Thomas Haller from comment #30) > > somehow NMManager must rise a nofify:p-c-metered signal when either the > > primary-connection changes or it's metered value. > > Pushed some fixups. How about renaming "PrimaryConnectionMetered" to "NMManager:metered"? In the documentation we can explain that this mostly corresponds to the metered property of the primary connection. But the concept of PrimaryConnection is already not so clearly defined. E.g. the IPv4 default-route could go along another device then the IPv6 route. So, in the future, it could make sense to combine the metered value from the best v4 and v6 device. The name "PrimaryConnectionMetered" would then be wrong. + if (priv->primary_connection) { + g_signal_handlers_disconnect_by_func (priv->primary_connection, + G_CALLBACK (connection_metered_changed), + self); + } g_clear_object (&priv->primary_connection); move clear_object() inside the block. »···signals[DEVICE_METERED_CHANGED] = »···»···g_signal_new (NM_ACTIVE_CONNECTION_DEVICE_METERED_CHANGED, »···»··· G_OBJECT_CLASS_TYPE (object_class), »···»··· G_SIGNAL_RUN_FIRST, I don't know why we predominantly have this indention style. I always add new signals/properties without the extra TAB. We should get a consistent style and stick with it (for new code). I'd vote from not using extra TAB. Seems danw likes the double TAB (looking at clients/tui).
(In reply to Thomas Haller from comment #32) > How about renaming "PrimaryConnectionMetered" to "NMManager:metered"? In the > documentation we can explain that this mostly corresponds to the metered > property of the primary connection. > > But the concept of PrimaryConnection is already not so clearly defined. E.g. > the IPv4 default-route could go along another device then the IPv6 route. > So, in the future, it could make sense to combine the metered value from the > best v4 and v6 device. The name "PrimaryConnectionMetered" would then be > wrong. Seems reasonable, I updated the property name. > move clear_object() inside the block. Fixed. > »···signals[DEVICE_METERED_CHANGED] = > »···»···g_signal_new (NM_ACTIVE_CONNECTION_DEVICE_METERED_CHANGED, > »···»··· G_OBJECT_CLASS_TYPE (object_class), > »···»··· G_SIGNAL_RUN_FIRST, > > I don't know why we predominantly have this indention style. I always add > new signals/properties without the extra TAB. We should get a consistent > style and stick with it (for new code). I'd vote from not using extra TAB. > Seems danw likes the double TAB (looking at clients/tui). I agree that we should conform to a common indentation style for the properties definition; OTOH adding new definitions with a style different from all the existing ones in the same file seems a bit ugly, so for now I'm not sure and I haven't changed it.
LGTM now. (pushed trivial whitespace fixup)
Looks good to me too.
Merged to master as 7d09debdf0b1bd6d76a645d5195e89e3ad16892f.
*** Bug 740982 has been marked as a duplicate of this bug. ***
*** Bug 761825 has been marked as a duplicate of this bug. ***
*** Bug 688216 has been marked as a duplicate of this bug. ***
*** Bug 725945 has been marked as a duplicate of this bug. ***