GNOME Bugzilla – Bug 765043
[review] dns: use DBus to make dnsmasq nameserver changes [th/dnsmasq-dbus-bgo765043]
Last modified: 2016-04-21 18:00:38 UTC
Mathieu sent patches to the mailing list to configure dnsmasq via D-Bus: https://mail.gnome.org/archives/networkmanager-list/2016-March/msg00144.html I took that patch (unmodified), and added a follow-up commit on top of it. Please review.
Created attachment 325996 [details] [review] fix SetServersEx parameters order Looks good to me (but of course I wrote the initial patch). I uploaded NM 1.1.93 to Ubuntu with all your fixups; we noticed there is one further patch missing (attached) because the SetServersEx message is not built correctly (it should be lists starting with the ip in all cases, and finishing with domain, not the other way around).
(In reply to Mathieu Trudel-Lapierre from comment #1) > Created attachment 325996 [details] [review] [review] > fix SetServersEx parameters order > > Looks good to me (but of course I wrote the initial patch). > > I uploaded NM 1.1.93 to Ubuntu with all your fixups; we noticed there is one > further patch missing (attached) because the SetServersEx message is not > built correctly (it should be lists starting with the ip in all cases, and > finishing with domain, not the other way around). Thanks. I took this patch and it's now on top of th/dnsmasq-dbus-bgo765043.
Maybe it would be useful to still write the server list to /var/run/NetworkManager/dnsmasq.conf, so that people have an easy way to see what's currently configured?
(In reply to Beniamino Galvani from comment #3) > Maybe it would be useful to still write the server list to > /var/run/NetworkManager/dnsmasq.conf, so that people have an easy way to see > what's currently configured? yeah, I can do that. ... but it's a mid-sized extra effort to restore that behavior. So, before start doing that, what is the opinions about this from others? (I personally think this is not necessary. Note that there is also a debug logging which prints those settings).
+ g_return_val_if_fail (priv->running, TRUE); g_return_val_if_fail (!priv->running, TRUE); priv->running is set only after the service is available. this means that while we are waiting for the name to appear on the bus, we will try to start a new dnsmasq instance every time update() is called. Instead of adding a new "appeared" signal to plugins and let the DNS manager handle it, an alternative would be to handle the delayed appearance entirely in the plugin... But at least, shouldn't we return a failure while waiting for the dbus name, so that resolv.conf is not updated with 127.0.0.1?
(In reply to Beniamino Galvani from comment #5) > + g_return_val_if_fail (priv->running, TRUE); > > g_return_val_if_fail (!priv->running, TRUE); > > > priv->running is set only after the service is available. this means > that while we are waiting for the name to appear on the bus, we will > try to start a new dnsmasq instance every time update() is called. How about the fixup? > Instead of adding a new "appeared" signal to plugins and let the DNS > manager handle it, an alternative would be to handle the delayed > appearance entirely in the plugin... Yeah, good point. I did that, see fixup. > But at least, shouldn't we return a failure while waiting for the dbus > name, so that resolv.conf is not updated with 127.0.0.1? Already on master, NM would configure 127.0.0.1 right away and not only when dnsmasq is ready to serve requests. So, there is not change in behavior there. But it seems complicated to properly implement what you suggest here, because even that the D-Bus name of dnsmasq is ready, doesn't mean that dnsmasq is fully configured either... The plugins would need a startup-state during which they are not yet ready. But once they are we would need a proper signal. That goes even further then the APPEARED signal that I just removed. So, removing the APPEARED signal is a step away from what you suggest here. I would not implement this. Pushed several new commits. * 3beaa39 dns: add and use _NMLOG() logging macro in nm-dns-plugin.c * 3b6b9bd dns: cleanup managing child process for NMDnsPlugin * 74ba415 dns: minor remove unused finalize() implementation from NMDnsPlugin * 0182f77 dns: minor code cleanup in NMDnsPlugin Please re-review
(In reply to Thomas Haller from comment #6) > > Instead of adding a new "appeared" signal to plugins and let the DNS > > manager handle it, an alternative would be to handle the delayed > > appearance entirely in the plugin... > > Yeah, good point. I did that, see fixup. Yes, it looks better now. > > But at least, shouldn't we return a failure while waiting for the dbus > > name, so that resolv.conf is not updated with 127.0.0.1? > > Already on master, NM would configure 127.0.0.1 right away and not only when > dnsmasq is ready to serve requests. So, there is not change in behavior > there. > But it seems complicated to properly implement what you suggest here, > because even that the D-Bus name of dnsmasq is ready, doesn't mean that > dnsmasq is fully configured either... The plugins would need a startup-state > during which they are not yet ready. But once they are we would need a > proper signal. That goes even further then the APPEARED signal that I just > removed. So, removing the APPEARED signal is a step away from what you > suggest here. > I would not implement this. Ok, makes sense. > Pushed several new commits. > * 3beaa39 dns: add and use _NMLOG() logging macro in nm-dns-plugin.c > * 3b6b9bd dns: cleanup managing child process for NMDnsPlugin > * 74ba415 dns: minor remove unused finalize() implementation from NMDnsPlugin > * 0182f77 dns: minor code cleanup in NMDnsPlugin Look good, pushed 2 trivial fixups.
merged to master: https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=497a8aa5c6463404200a3fcc745aa65396dc4f22
There was a minor regression. Fixed now with: https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=3d3f71acec5c59a8e01a4d1b0897183fbc49f57c