Re: Working with a local DNS cache

[Dan Williams <dcbw redhat com>]

On Fri, 2009-08-14 at 13:12 -0700, Adam Langley wrote:
> On Fri, Aug 7, 2009 at 4:45 PM, Adam Langley<agl imperialviolet org> wrote:
> > It appears that I need to make the IP config code asynchronous as a
> > first step. I'll start hacking on that.
> 
> Please see the commits at http://github.com/agl/NetworkManager/commits/agl
> 
> (is that a good way to post patches? If not, what would folks prefer?)
> 
> I did a couple of refactoring commits ([1][2]) which just make it
> easier to add a DBus call in the right place. Then the 3rd commit[3]
> adds support for org.chromium.local-dns-cache. As you mentioned, we
> probably want to use the dnsmasq interface[4], it's just that I happen
> to have a caching server which implements org.chromium.local-dns-cache
> and so I used that for testing. If/when you're happy with the code
> I'll spin up dnsmasq and make sure it works with that.

Well, Marcel had some valid issues with dnsmasq, and others may want to
use bind, so in the end we probably want a GInterface for this and then
have classes that implement the GInterface for each of the specific
caching name daemons.  I don't really have a problem with that; we'll
probably have to add arguments and a config file option for which one
you want to use, because people may want to use dnsmasq underneath
chromium too and have chromium still use 127.0.0.1.

> I'm not really familiar with glib coding styles so please forgive and
> correct any incorrect mannerisms.

No problem; everyone has their own.  A few of the guidelines are
documented in the CONTRIBUTING but I see now that I should extend that
significantly.

A few style comments...

Keep a space between the function name and the opening '(' (patch #2,
new line 502 + 505 + etc).  Also keep C-style comments (except for
FIXMEs, stupid I know).  Please also use inet_pton/inet_ntop instead of
inet_ntoa (3rd patch new line 549).  Next, I generally try to keep
assignments in the variable declaration area pretty short (just
constants or other variables, not function results), so stuff like this:

+  NMNamedManagerPrivate *priv = NM_NAMED_MANAGER_GET_PRIVATE (mgr);
+  NMDBusManager *dbus_mgr = nm_dbus_manager_get();
+  priv->has_dbus_resolver =
+    nm_dbus_manager_name_has_owner(dbus_mgr, LOCAL_CACHE_DBUS_NAME);

should be:

  NMNamedManagerPrivate *priv = NM_NAMED_MANAGER_GET_PRIVATE (mgr);
  NMDBusManager *dbus_mgr;

  dbus_mgr = nm_dbus_manager_get();
  priv->has_dbus_resolver = nm_dbus_manager_name_has_owner (dbus_mgr, LOCAL_CACHE_DBUS_NAME);

80 cols is also a guide not a rule, so if looks like ass to wrap the
line (especially on assignments) then just go over 80 cols.

Next, variable names should be lower-case with _ as the separator
instead of kMaxNameservers; in the case of kMaxNameservers which is a
constant, it should be #defined somewhere above like so:

#define MAX_NAMESERVERS 16

static gboolean
dispatch_local_cache (NMNamedManager *mgr, NMResolvConfData *rc,
            NMResolvConfCallback callback, void *data)
{
  NMDBusManager *dbus_manager = nm_dbus_manager_get();
  DBusGConnection *conn = nm_dbus_manager_get_connection(dbus_manager);
  DBusGProxy *proxy;
  guint32 ips_uint32[MAX_NAMESERVERS];

Overall, approach looks pretty good.  I wont' subject you to
GInterface/GObject if you don't volunteer for it (though I'd be happy to
guide) so let me know when you've got the style cleanups and I'll run
through again.  Thanks!

Dan