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 764689 - [review] bg/dns-manager-vpn-bgo764689 - Handle multiple VPN configurations in DNS manager
[review] bg/dns-manager-vpn-bgo764689 - Handle multiple VPN configurations in...
Status: RESOLVED FIXED
Product: NetworkManager
Classification: Platform
Component: IP and DNS config
git master
Other Linux
: Normal normal
: ---
Assigned To: NetworkManager maintainer(s)
NetworkManager maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2016-04-06 15:27 UTC by Beniamino Galvani
Modified: 2016-04-18 11:29 UTC
See Also:
GNOME target: ---
GNOME version: ---


Description Beniamino Galvani 2016-04-06 15:27:26 UTC 
When multiple VPNs are active, the DNS manager considers all the VPN configurations except one as normal configurations and thus they might not be given a higher priority. Steps to reproduce:

 1. activate VPN1
 2. activate VPN2
 3. activate connection1

Now resolv.conf contains:

 nameserver of VPN1
 nameserver of connection1
 nameserver of VPN2

Instead the two VPN nameserver should be at the top. Maybe we should also decide their order based on some other parameters?
Comment 1 Beniamino Galvani 2016-04-06 15:30:09 UTC 
Fix in branch bg/dns-manager-vpn-bgo764689, please review.
Comment 2 Thomas Haller 2016-04-07 11:54:10 UTC 
+static inline NMDnsManagerPrivate *
+NM_DNS_MANAGER_GET_PRIVATE (NMDnsManager *self)
+{
+    g_assert (NM_IS_DNS_MANAGER (self));
+    return self->priv;
+}


realistically, nobody is ever going to do a production build of NM with G_DISABLE_ASSERT. Hence, you always check the type.

For me, the selling point of using a private pointer it to avoid the type lookup involved with G_TYPE_INSTANCE_GET_PRIVATE(). If you always check the type, there is not much point in doing it.

Well, you cannot have it both. Either you pay for the check, or you hope for the best.

I think nm_assert() would be preferably, because that one is a NOP on production builds.




the switch in nm_dns_manager_add_ip4_config(), maybe it should have a g_return_if_reached() for "default"?



maybe reorder the if-else-if in nm_dns_manager_remove_ip4_config()? I'd expect to see more device-configs then vpn configs there...



Rest lgtm
Comment 3 Beniamino Galvani 2016-04-11 12:52:58 UTC 
(In reply to Thomas Haller from comment #2)

> For me, the selling point of using a private pointer it to avoid the type
> lookup involved with G_TYPE_INSTANCE_GET_PRIVATE(). If you always check the
> type, there is not much point in doing it.
> 
> Well, you cannot have it both. Either you pay for the check, or you hope for
> the best.

I was interested more in having priv available when debugging. Now I've removed the check and turned the function to a macro that returns (o)->priv.

> the switch in nm_dns_manager_add_ip4_config(), maybe it should have a
> g_return_if_reached() for "default"?

Fixed.

> maybe reorder the if-else-if in nm_dns_manager_remove_ip4_config()? I'd
> expect to see more device-configs then vpn configs there...

Done and repushed.
Comment 4 Thomas Haller 2016-04-11 13:02:31 UTC 
(In reply to Beniamino Galvani from comment #3)
> (In reply to Thomas Haller from comment #2)
> 
> > For me, the selling point of using a private pointer it to avoid the type
> > lookup involved with G_TYPE_INSTANCE_GET_PRIVATE(). If you always check the
> > type, there is not much point in doing it.
> > 
> > Well, you cannot have it both. Either you pay for the check, or you hope for
> > the best.
> 
> I was interested more in having priv available when debugging. Now I've
> removed the check and turned the function to a macro that returns (o)->priv.

Ah ok. Makes sense. I don't have a preference over inline-function vs. macro :)

> Done and repushed.

lgtm