Created attachment 210718 [details] [review]
Add support for MX and SOA records to GResolver

 * Adds GResolver functions for looking up MX and SOA
   records, via a general vfunc which so the resolver
   backend can be easily extended to support further
   record types.
 * Adds boxed types GMxRecord and GSOARecord
 * Rework the resolver test so that it has support for
   looking up MX and SOA records, and uses GOptionContext

Public *classes* for MX and SOA records?

My knee-jerk on this is 'no way'.

(In reply to comment #2)
> Public *classes* for MX and SOA records?
> 
> My knee-jerk on this is 'no way'.

They're boxeds, not classes.  However, it does seem to me like we might want something a little more "data blob" oriented.  Basically if your data is read only, you should really consider using GVariant.

Yeah, you lose a bit of type safety passing around GVariant * in a lot of places, but it's also a lot nicer in many ways than defining tons of C structures and GTypes.

This patch is clearly following what already exists in GSrvTarget though.

Created attachment 210731 [details] [review]
I attached the wrong patch. This one includes support for TXT

(In reply to comment #3)
> They're boxeds, not classes.  However, it does seem to me like we might want
> something a little more "data blob" oriented.  Basically if your data is read
> only, you should really consider using GVariant.

That's a good idea. We would just document the signatures. Using a single GVariant return type for these less used DNS records also lets us keep a simple GResolver API. I'll update the patch for this tomorrow.

These functions aren't used that often, so it's not a problem to have accessing the data returned a little more complex. But when these functions *are* needed, they'll be an absolute life saver, given that there's no glibc API for DNS records like TXT or MX. You have to parse the output of res_query on your own :S , not to mention cross platform issues...

> Yeah, you lose a bit of type safety passing around GVariant * in a lot of
> places, but it's also a lot nicer in many ways than defining tons of C
> structures and GTypes.

In this case it makes sense. 

> This patch is clearly following what already exists in GSrvTarget though.

Right, it seems there's lots of strange little structs hanging around in glib. But I understand if we don't want to keep going that way.

Not sure I love the idea of using GVariant like that either...

Can't we just make these things be private with their public interface as a GSocketConnectable implementation?  I guess that's not so great for TXT records, but do we really need those?

Created attachment 210772 [details] [review]
Now using GVariant for representing records

Add support for MX TXT and SOA records to GResolver

 * Add resolver functions for looking up DNS records of
   various types. Currently implemented: MX, TXT, SOA, SRV
 * Return records as GVariant tuples.
 * Make the GSrvTarget lookups a wrapper over this new
   functionality.
 * Rework the resolver test so that it has support for
   looking up MX, SOA, TXT records, and uses GOptionContext

(In reply to comment #6)
> Not sure I love the idea of using GVariant like that either...

There's lots about glib that only a mother could love :) .. COUGH floating-references, COUGH COUGH

> Can't we just make these things be private with their public interface as a
> GSocketConnectable implementation?  I guess that's not so great for TXT
> records, but do we really need those?

Well the point here is that we're doing discovery and setup of accounts (like email, kerberos, etc...) using DNS. We're using Glib in a lot more system dbus services. And for those purposes you actually need to look at what's in DNS, not just connect.

The latest implementation (Colin's suggestion) only adds 3 functions and an enum to gio. The alternatives aren't super appealing:

 * Using nslookup and screenscraping the results (it doesn't even return a
   non-zero exit code on failure).
 * Or 'egging' GThreadedResolver into other projects, and implementing these
   additions in the copy, each project dealing with parsing raw DNS responses,
   and cross platform issues.

(In reply to comment #0)
> Apparently the MX stuff is useful to mbarnes...

Turns out what I really needed was NS records, although MX records have that information too.  My use case is autodiscovery of mail account information, where the user provides an email address and Evolution figures out the rest.  Currently I'm calling res_query() and parsing the answer myself, so any support from GLib would be much appreciated.

Created attachment 210779 [details] [review]
Here you go. Now with NS support.

Add support for MX, TXT, NS and SOA records to GResolver

 * Add resolver functions for looking up DNS records of
   various types. Currently implemented: MX, TXT, SOA, SRV, NS
 * Return records as GVariant tuples.
 * Make the GSrvTarget lookups a wrapper over this new
   functionality.
 * Rework the resolver test so that it has support for
   looking up MX, NS, SOA, TXT records, and uses GOptionContext

(In reply to comment #6)
> Not sure I love the idea of using GVariant like that either...
> 
> Can't we just make these things be private with their public interface as a
> GSocketConnectable implementation?  I guess that's not so great for TXT
> records, but do we really need those?

As Stef noted, this is not about connecting, so that won't work.

Mapping DNS results to GVariant actually makes a lot of sense. You could
*almost* write a function that takes a raw DNS response and a GVariantType, and
parses the DNS response directly without needing any specialized
per-record-type knowledge. (Actually, if you declared that 's' means an
arbitrary string and 'o' means a DNS name, then MX is '(qo)', SOA is
'(oouuuu)', TXT is 's', NS is 'o', SRV is '(oqquqqqqo)', etc. But there are
other RRs that aren't as easily represented though. AAAA is a single 128-bit
uint, TSIG has a 48-bit uint in it, etc.)

(In reply to comment #11)
> Mapping DNS results to GVariant actually makes a lot of sense. You could
> *almost* write a function that takes a raw DNS response and a GVariantType, and
> parses the DNS response directly without needing any specialized
> per-record-type knowledge.

(Though we couldn't actually implement things this way, because Windows doesn't have a function that returns unparsed DNS data.)

Comment on attachment 210779 [details] [review]
Here you go. Now with NS support.

>diff --git a/docs/reference/gio/gio-sections.txt b/docs/reference/gio/gio-sections.txt

you need to add g_resolver_record_type_get_type too (in the <SUBSECTION Private> section of GResolver, so it doesn't actually end up in the docs).

>+ * The type of record that g_resolver_lookup_records() or
>+ * g_resolver_lookup_records_async() should retrieve. The records are returned
>+ * as lists of #GVariant tuples.

That seems like the right thing to me, since it's consistent with the other methods, but I'll point this out just in case any else wants to object and say it should return a GVariant array.

+ * %G_RESOLVER_RECORD_SRV records are returned as variants with the signature
+ * '(sqqq)', containing a string of the hostname, a guint16 with the priority,
+ * and a guint16 with the weight, a guint16 with the port.
+ *
+ * %G_RESOLVER_RECORD_MX records are returned as variants with the signature
+ * '(sq)', representing a string containing the mail exchanger, and a guint16

Would it be better to say that the variant data always appears in the same order that the data appears in the DNS record? ('(qqqs)' and '(qs)' here)

Also, the "and" is in the wrong place in the SRV paragraph.

>+ * %G_RESOLVER_RECORD_TXT records are returned as variants with the signature
>+ * '(as)'
>+ * %G_RESOLVER_RECORD_NS records are returned as variants with the signature
>+ * '(s)'

should they be 'as' and 's' instead of '(as)' and '(s)'? I'm not sure...

> /**
>+ * g_resolver_lookup_mx_records:

s/mx_//

>+g_resolver_lookup_records (GResolver           *resolver,
>+                           const gchar         *rrname,
>+                           GResolverRecordType  record_type,
>+                           GCancellable        *cancellable,
>+                           GError             **error)

nitpick: there should be an additional space between the types and the names there (so that there's one completely empty column all the way down).

>+gint
>+_g_resolver_record_type_to_rrtype (GResolverRecordType type)

The unix values (T_SRV, etc) are by definition the same as the DNS "TYPE" values, and the win32 values (DNS_TYPE_SRV, etc) appear to be the same as well. So you could instead just define the enum as:

    typedef enum {
      G_RESOLVER_RECORD_SRV = 33,
      G_RESOLVER_RECORD_MX  = 15,
      etc

Or alternatively, extract the system values at configure time like we do with GSocketFamily and GSocketMsgFlags.

>           errnum = G_RESOLVER_ERROR_NOT_FOUND;
>-          format = _("No service record for '%s'");
>+          format = _("No DNS record for '%s'");

That's not strictly true. There may be a record for %s, just not of the requested type.

It's probably not worth having separate error strings for each type. Just add "of the requested type" to the error string.

>+      /* To silence gcc warnings */
>+      namebuf[0] = namebuf[1];

what warning, and why does that silence it?

>+do_lookup_records (GThreadedResolverRequest *req,

>   guchar answer[1024];

possibly will need to be bigger for some TXT records, etc? And/or I guess maybe we need to check the return value of res_query to see if it overflowed and we need to try again with a larger buffer? (I'm not sure what res_query does on overflow. The man page doesn't say.)

>+lookup_records (GResolver           *resolver,
>+                const gchar         *rrname,
>+                GResolverRecordType record_type,
>+                GCancellable       *cancellable,
>+                GError            **error)

alignment issues again

>+  for (l = req->u.records.results; l != NULL; l = g_list_next (l))
>+    g_variant_unref (l->data);

could just use g_list_free_full() like your docs suggested before

>+          printf ("%s (pri %u)\n", hostname, (guint32)priority);

not that it matters on any real platforms, but the cast for %u should be to guint.

>+print_resolved_txt (const char *rrname,

should probably print a blank line or something between each variant? (to distinguish a response with two single-element arrays from a response with a single two-element array, etc)

should we return MX records in sorted order? (Likewise SRV records when they're fetched via lookup_records?)

Created attachment 210991 [details] [review]
Updated for Dan's review.

(In reply to comment #13)
> (From update of attachment 210779 [details] [review])
> >diff --git a/docs/reference/gio/gio-sections.txt b/docs/reference/gio/gio-sections.txt
> 
> you need to add g_resolver_record_type_get_type too (in the <SUBSECTION
> Private> section of GResolver, so it doesn't actually end up in the docs).

Done.

> >+ * The type of record that g_resolver_lookup_records() or
> >+ * g_resolver_lookup_records_async() should retrieve. The records are returned
> >+ * as lists of #GVariant tuples.
> 
> That seems like the right thing to me, since it's consistent with the other
> methods, but I'll point this out just in case any else wants to object and say
> it should return a GVariant array.

Yeah, was thinking the same thing. But I came down in favor of lists. Allows the caller to easily sort, hash and easily keep the records individually.

> + * %G_RESOLVER_RECORD_SRV records are returned as variants with the signature
> + * '(sqqq)', containing a string of the hostname, a guint16 with the priority,
> + * and a guint16 with the weight, a guint16 with the port.
> + *
> + * %G_RESOLVER_RECORD_MX records are returned as variants with the signature
> + * '(sq)', representing a string containing the mail exchanger, and a guint16
> 
> Would it be better to say that the variant data always appears in the same
> order that the data appears in the DNS record? ('(qqqs)' and '(qs)' here)

Whoops. Yes that's a good plan. I had meant to do that, but forgot.

> Also, the "and" is in the wrong place in the SRV paragraph.
> 
> >+ * %G_RESOLVER_RECORD_TXT records are returned as variants with the signature
> >+ * '(as)'
> >+ * %G_RESOLVER_RECORD_NS records are returned as variants with the signature
> >+ * '(s)'
> 
> should they be 'as' and 's' instead of '(as)' and '(s)'? I'm not sure...

Strangely enough TXT records have multiple strings per record, and NS records only have one host each.

> > /**
> >+ * g_resolver_lookup_mx_records:
> 
> s/mx_//
> 
> >+g_resolver_lookup_records (GResolver           *resolver,
> >+                           const gchar         *rrname,
> >+                           GResolverRecordType  record_type,
> >+                           GCancellable        *cancellable,
> >+                           GError             **error)
> 
> nitpick: there should be an additional space between the types and the names
> there (so that there's one completely empty column all the way down).

:)

> >+gint
> >+_g_resolver_record_type_to_rrtype (GResolverRecordType type)
> 
> The unix values (T_SRV, etc) are by definition the same as the DNS "TYPE"
> values, and the win32 values (DNS_TYPE_SRV, etc) appear to be the same as well.
> So you could instead just define the enum as:
> 
>     typedef enum {
>       G_RESOLVER_RECORD_SRV = 33,
>       G_RESOLVER_RECORD_MX  = 15,
>       etc
> 
> Or alternatively, extract the system values at configure time like we do with
> GSocketFamily and GSocketMsgFlags.

I tried this. Oh god. I gave up somewhere between defining an m4 system, and
wondering why windns.h isn't included in anything but iphlpapi.h. Anyhow, I
think it's a lot cleaner to just have some switches than it is to screw around
in bowels of configure. Less code anyway. Plus the way GSocketFamily is done is
so annoying when cross compiling.

> >           errnum = G_RESOLVER_ERROR_NOT_FOUND;
> >-          format = _("No service record for '%s'");
> >+          format = _("No DNS record for '%s'");
> 
> That's not strictly true. There may be a record for %s, just not of the
> requested type.
> 
> It's probably not worth having separate error strings for each type. Just add
> "of the requested type" to the error string.
> 
> >+      /* To silence gcc warnings */
> >+      namebuf[0] = namebuf[1];

Done.

> what warning, and why does that silence it?

namebuf is read but not 'used'.

> >+do_lookup_records (GThreadedResolverRequest *req,
> 
> >   guchar answer[1024];
> 
> possibly will need to be bigger for some TXT records, etc? And/or I guess maybe
> we need to check the return value of res_query to see if it overflowed and we
> need to try again with a larger buffer? (I'm not sure what res_query does on
> overflow. The man page doesn't say.)

On overflow, linux seems to return the input buffer length. Solaris man pages
say that it returns the length needed. BSD no docs. Anyway, I put in some code
that should work for both.

> >+lookup_records (GResolver           *resolver,
> >+                const gchar         *rrname,
> >+                GResolverRecordType record_type,
> >+                GCancellable       *cancellable,
> >+                GError            **error)
> 
> alignment issues again

Fixed.
> 
> >+  for (l = req->u.records.results; l != NULL; l = g_list_next (l))
> >+    g_variant_unref (l->data);
> 
> could just use g_list_free_full() like your docs suggested before

Done.

> >+          printf ("%s (pri %u)\n", hostname, (guint32)priority);
> 
> not that it matters on any real platforms, but the cast for %u should be to
> guint.

Done.

> >+print_resolved_txt (const char *rrname,
> 
> should probably print a blank line or something between each variant? (to
> distinguish a response with two single-element arrays from a response with a
> single two-element array, etc)

Done.

(In reply to comment #14)

> should we return MX records in sorted order? (Likewise SRV records when they're
> fetched via lookup_records?)

I'd rather not. The g_resolver_lookup_records() interface is (and should be) pretty raw and uninterpreted in my opinion. So I won't do this unless you insist :)

(In reply to comment #16)
> > >+ * %G_RESOLVER_RECORD_TXT records are returned as variants with the signature
> > >+ * '(as)'
> > >+ * %G_RESOLVER_RECORD_NS records are returned as variants with the signature
> > >+ * '(s)'
> > 
> > should they be 'as' and 's' instead of '(as)' and '(s)'? I'm not sure...
> 
> Strangely enough TXT records have multiple strings per record, and NS records
> only have one host each.

No, that's not what I was saying. Currently you have TXT as '(as)', which is a one-element tuple whose single member is an array of strings. I was asking if it would make more sense to have it be just 'as', since, unlike in the SOA, SRV, and MX cases, the enclosing tuple is completely superfluous.

I can see an argument for being consistent and always having the tuple, but then again, it's sort of the equivalent to returning a struct containing a char**, instead of just returning a char** directly.

(In reply to comment #17)
> (In reply to comment #16)
> > > >+ * %G_RESOLVER_RECORD_TXT records are returned as variants with the signature
> > > >+ * '(as)'
> > > >+ * %G_RESOLVER_RECORD_NS records are returned as variants with the signature
> > > >+ * '(s)'
> > > 
> > > should they be 'as' and 's' instead of '(as)' and '(s)'? I'm not sure...
> > 
> > Strangely enough TXT records have multiple strings per record, and NS records
> > only have one host each.
> 
> No, that's not what I was saying. Currently you have TXT as '(as)', which is a
> one-element tuple whose single member is an array of strings. I was asking if
> it would make more sense to have it be just 'as', since, unlike in the SOA,
> SRV, and MX cases, the enclosing tuple is completely superfluous.

Ah, makes sense. Sorry, late at night for me so things are getting blurry :)

> I can see an argument for being consistent and always having the tuple, but
> then again, it's sort of the equivalent to returning a struct containing a
> char**, instead of just returning a char** directly.

In that analogy you have the compiler on your side :) So I'd prefer consistency here (using the tuple regardless). There's a weak precedent too: When you send a GDbus message using a single string arg, you do it like "(s)".

Comment on attachment 210991 [details] [review]
Updated for Dan's review.

looks good, though it would be better if you could fix the tabs-vs-spaces issues, at least within existing code. eg:

>+GList *_g_resolver_records_from_res_query  (const gchar      *rrname,
>+                                            gint              rrtype,
> 					    guchar           *answer,

Created attachment 211304 [details] [review]
Cleaned up whitespace, will merge after branch for 2.33.x

Add support for MX, TXT, NS and SOA records to GResolver

 * Add resolver functions for looking up DNS records of
   various types. Currently implemented: MX, TXT, SOA, SRV, NS
 * Return records as GVariant tuples.
 * Make the GSrvTarget lookups a wrapper over this new
   functionality.
 * Rework the resolver test so that it has support for
   looking up MX, NS, SOA, TXT records, and uses GOptionContext

Merged with glib master, now that glib-2-32 branch has taken place.