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 761371 - [review] device: add ext_ip6_config_captured to hold last-read IP configuration [th/device-ext-ip6-config-captured-bgo761371]
[review] device: add ext_ip6_config_captured to hold last-read IP configurati...
Status: RESOLVED FIXED
Product: NetworkManager
Classification: Platform
Component: general
git master
Other Linux
: Normal normal
: ---
Assigned To: NetworkManager maintainer(s)
NetworkManager maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2016-01-31 18:19 UTC by Thomas Haller
Modified: 2016-02-02 10:59 UTC
See Also:
GNOME target: ---
GNOME version: ---


Description Thomas Haller 2016-01-31 18:19:31 UTC 
I think that http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=2d1638bba95fa19f2e20caf016460eaecc8ee496 is not entirely correct (neither was the previous version).


ip6_config is the combined intend of what should be configured on the device.
ext_ip6_config is what is actually configured on the device but not in any of the other ip6-config instances (thus, the truly externally configured parts).


Please review
Comment 1 Beniamino Galvani 2016-02-02 10:31:48 UTC 
> device: add ext_ip6_config_captured to remember the last-read platform configuration

	NMIP6Config *  ext_ip6_config; /* Stuff added outside NM */
+       NMIP6Config *  ext_ip6_config_captured; /* Stuff added outside NM */

Change to /* Last captured platform configuration */ or something similar ?


> device: move have_ip6_address() to nm_ip6_config_get_address_first_nontentative()

+       if (   ((!!IN6_IS_ADDR_LINKLOCAL (&addr->address)) == linklocal)

The !! is not needed, IN6_IS_ADDR_LINKLOCAL already returns [0,1].

Otherwise LGTM.
Comment 2 Thomas Haller 2016-02-02 10:54:53 UTC 
(In reply to Beniamino Galvani from comment #1)
> > device: add ext_ip6_config_captured to remember the last-read platform configuration
> 
> 	NMIP6Config *  ext_ip6_config; /* Stuff added outside NM */
> +       NMIP6Config *  ext_ip6_config_captured; /* Stuff added outside NM */
> 
> Change to /* Last captured platform configuration */ or something similar ?

reworded.

> > device: move have_ip6_address() to nm_ip6_config_get_address_first_nontentative()
> 
> +       if (   ((!!IN6_IS_ADDR_LINKLOCAL (&addr->address)) == linklocal)
> 
> The !! is not needed, IN6_IS_ADDR_LINKLOCAL already returns [0,1].

which we both know by looking at our particular implementation of "in.h".

The manual page inet(3N) reads:

  The IN6_IS_ADDR_* macros evaluate to true (non zero) if true, otherwise they 
  evaluate to 0. 

does that mean true equals "1"? Maybe... but let's just be explict about that.
Comparing boolean integers with == seems always shaky to me (in C).