Bug 23920

Summary: Should use GResolver instead of /GibberResolver
Product: Telepathy Reporter: Guillaume Desmottes <guillaume.desmottes>
Component: gabbleAssignee: Mike Ruprecht <cmaiku>
Status: RESOLVED FIXED QA Contact: Telepathy bugs list <telepathy-bugs>
Severity: normal    
Priority: medium CC: cmaiku, sjoerd
Version: unspecified   
Hardware: Other   
OS: All   
Whiteboard:
i915 platform: i915 features:

Description Guillaume Desmottes 2009-09-14 03:30:46 UTC
As said in bug #23685, Gabble should use gio's GResolver instead of its own GibberResolver.
That means Gabble will depend on a recent gio but that's fine as it already (indirectly) does as Wocky needs it.
Comment 1 Mike Ruprecht 2009-11-20 15:07:30 UTC
Here is my branch to fix this bug:

http://git.collabora.co.uk/?p=user/maiku/telepathy-gabble.git;a=shortlog;h=refs/heads/gresolver
Comment 2 Sjoerd Simons 2009-11-30 05:04:47 UTC
9034d0c67a7f2928490f2640f41e8db9c6d13bac:
  Do you cope with the semantic change that when the factory is disposed with GibberResolver the callback isn't called, while with the GResolver the operation is cancelled but the callback is still called ?

e757c00715fcdc728913d985676515b3784cae20:
+#ifdef G_LOG_DOMAIN
+#undef G_LOG_DOMAIN
+#endif
+#define G_LOG_DOMAIN "test-resolver"

uh what now? should probably use the standard gabble/wocky debug framework..

+enum
+{
+  PROP_REAL_RESOLVER = 1,
+};

It should never chain up to a real resolver, as that would mean the test could depend on the network.

+typedef struct _fake_host { char *key; char *addr; } fake_host;
+typedef struct _fake_serv { char *key; GSrvTarget *srv; } fake_serv;

Not telepathy style..

+static gchar *
+_service_rrname (const char *service,
+                 const char *protocol,
+                 const char *domain)
+{
+  gchar *rrname, *ascii_domain = NULL;
+
+  if (g_hostname_is_non_ascii (domain))
+    domain = ascii_domain = g_hostname_to_ascii (domain);
+
+  rrname = g_strdup_printf ("_%s._%s.%s", service, protocol, domain);
+
+  g_free (ascii_domain);
+  return rrname;
+}

not bothering with g_h_is_non_ascii would probably make the code a bit nicer

++static GList *
+find_fake_services (TestResolver *tr, const char *name)
use gchar instead of char
+{
+  GList *fake = NULL;
+  GList *rval = NULL;
+
+  for (fake = tr->fake_SRV; fake; fake = fake->next)
  Use g_list_next and fake != NULL;

+    {
+      fake_serv *entry = fake->data;
+      if (entry && !g_strcmp0 (entry->key, name))
 Can't we assert entry != NULL and if not use entry != NULL, not entry

+          rval = g_list_append (rval, g_srv_target_copy (entry->srv));
+    }
+  return rval;
+}
+
+static GList *
+find_fake_hosts (TestResolver *tr, const char *name)
+{
+  GList *fake = NULL;
+  GList *rval = NULL;
+
+  for (fake = tr->fake_A; fake; fake = fake->next)
+    {
+      fake_host *entry = fake->data;
+      if (entry && !g_strcmp0 (entry->key, name))
+        rval =
+          g_list_append (rval, g_inet_address_new_from_string (entry->addr));
+    }
+  return rval;
+}
Same comment as in find_fake_services

+static void
+lookup_service_async (GResolver *resolver,
+    const char *rr,
+    GCancellable *cancellable,
+    GAsyncReadyCallback  cb,
+    gpointer data)
+{
+  GError *error = NULL;
+  TestResolver *tr = TEST_RESOLVER (resolver);
+  GList *addr = find_fake_services (tr, rr);
+  GObject *source = G_OBJECT (resolver);
+  GSimpleAsyncResult *res = NULL;
+#ifdef DEBUG_FAKEDNS
+  GList *x;
+#endif

debugging shouldn't be a special compile time function, should probably use the same debugging infrastructure as in wocky/gabble if we want any debugging in it (probably not needed)

+  if (addr == NULL)
+    {
+      GResolver *real = G_RESOLVER (tr->real_resolver);
+      addr = G_RESOLVER_GET_CLASS (real)->
+        lookup_service (real, rr, cancellable, &error);
Don't chain up to a real resolver. we've fixed this in wocky, but not done some other cleanup yet, a resync might be good

+    }
+#ifdef DEBUG_FAKEDNS
+  else
+    for (x = addr; x; x = x->next)
+      g_debug ("FAKE SRV: addr: %s; port: %d; prio: %d; weight: %d;\n",
+          g_srv_target_get_hostname ((GSrvTarget *) x->data),
+          g_srv_target_get_port ((GSrvTarget *) x->data),
+          g_srv_target_get_priority ((GSrvTarget *) x->data),
+          g_srv_target_get_weight ((GSrvTarget *) x->data));
+#endif
+
+  if (addr != NULL)
+      res = g_simple_async_result_new (source, cb, data, lookup_service_async);
+  else
+      res = g_simple_async_result_new_from_error (source, cb, data, error);
+
+  g_simple_async_result_set_op_res_gpointer (res, addr, NULL);

The code would be a lot easier to understnad if it did:
  res = g_s_a_r_new ()
  if (addr != NULL)
    g_s_a_r_set_op_res_g ()
  else
    g_s_a_r_set_error ()

+  g_simple_async_result_complete (res);
should be complete_in_idle 

+static GList *
+lookup_service_finish (GResolver *resolver,
+                       GAsyncResult *result,
+                       GError **error)
+{
+  GList *res = NULL;
+  GSimpleAsyncResult *simple = G_SIMPLE_ASYNC_RESULT (result);
+  g_simple_async_result_propagate_error (simple, error);
+  res = g_simple_async_result_get_op_res_gpointer (simple);

Would be better to use the normal boilerplate for this.. Also don't assme get_op_res_gpointer is NULL when an error is set..

+static void
+lookup_by_name_async (GResolver *resolver,
+                      const gchar *hostname,
+                      GCancellable *cancellable,
+                      GAsyncReadyCallback  cb,
+                      gpointer data)
+{
+  GError *error = NULL;
+  TestResolver *tr = TEST_RESOLVER (resolver);
+  GList *addr = find_fake_hosts (tr, hostname);
+  GObject *source = G_OBJECT (resolver);
+  GSimpleAsyncResult *res = NULL;
+#ifdef DEBUG_FAKEDNS
+  GList *x;
+  char a[32];
+#endif
+
+  if (addr == NULL)
+    {
+      GResolver *real = G_RESOLVER (tr->real_resolver);
+      addr = g_resolver_lookup_by_name (real, hostname, cancellable, &error);
         don't chain up..
+    }
+#ifdef DEBUG_FAKEDNS
+  else
+    for (x = addr; x; x = x->next)
+      g_debug ("FAKE HOST: addr: %s;\n",
+          inet_ntop (AF_INET,
+              g_inet_address_to_bytes (x->data), a, sizeof (a)));
+#endif
more debugging crap...

+  if (addr != NULL)
+      res = g_simple_async_result_new (source, cb, data, lookup_service_async);
+  else
+      res = g_simple_async_result_new_from_error (source, cb, data, error);
+
+  g_simple_async_result_set_op_res_gpointer (res, addr, NULL);

Same comments about laying out the code as above

+static GList *
+lookup_by_name_finish (GResolver *resolver,
+    GAsyncResult *result,
+    GError **error)
+{
+  GList *res = NULL;
+  GSimpleAsyncResult *simple = G_SIMPLE_ASYNC_RESULT (result);
+  g_simple_async_result_propagate_error (simple, error);
+  res = g_simple_async_result_get_op_res_gpointer (simple);
+  return res;

same comment as above, use the normal boilerplate, don't mak assumption about op_res_pointer

+static void
+test_resolver_class_init (TestResolverClass *klass)
+{
+  GResolverClass *resolver_class = G_RESOLVER_CLASS (klass);
+  GObjectClass *object_class = G_OBJECT_CLASS (klass);
+  GParamSpec *spec;
+
+  object_class->set_property = test_resolver_set_property;
+  object_class->get_property = test_resolver_get_property;
+
+  spec = g_param_spec_object ("real-resolver", "real-resolver",
+      "The real resolver to use when we don't have a kludge entry",
+      G_TYPE_RESOLVER,
+      G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS | G_PARAM_CONSTRUCT);

die real-resolver, die :) also means we can get rid of the getter and setter

+void
+test_resolver_reset (TestResolver *tr)
+{
+  GList *fake = NULL;
+
+  for (fake = tr->fake_A; fake; fake = fake->next)
use fake != NULL, and use g_list_next.
+    {
+      fake_host *entry = fake->data;
+      g_free (entry->key);
+      g_free (entry->addr);
+      g_free (entry);
+    }
+  g_list_free (tr->fake_A);
+  tr->fake_A = NULL;
+
+  for (fake = tr->fake_SRV; fake; fake = fake->next)
ditto
+    {
+      fake_serv *entry = fake->data;
+      g_free (entry->key);
+      g_srv_target_free (entry->srv);
+      g_free (entry);
+    }
+  g_list_free (tr->fake_SRV);
+  tr->fake_SRV = NULL;
+}
Comment 3 Mike Ruprecht 2009-12-01 20:07:51 UTC
I updated the branch per your review.

(In reply to comment #2)
> 9034d0c67a7f2928490f2640f41e8db9c6d13bac:
>   Do you cope with the semantic change that when the factory is disposed with
> GibberResolver the callback isn't called, while with the GResolver the
> operation is cancelled but the callback is still called ?

Yes. My understanding is that when the operation is cancelled, the callback returns with no entries and an error set along the lines of the operation having been cancelled. Because of that, it's caught in the normal error handling code.
Comment 4 Sjoerd Simons 2009-12-02 04:22:59 UTC
0bf47895312850b1a3434cae87e5c4a1d564108e:

You should still use g_hostname_to_ascii but i would just call it unconditionally.
Comment 5 Sjoerd Simons 2009-12-02 04:26:04 UTC
(In reply to comment #3)
> I updated the branch per your review.
> 
> (In reply to comment #2)
> > 9034d0c67a7f2928490f2640f41e8db9c6d13bac:
> >   Do you cope with the semantic change that when the factory is disposed with
> > GibberResolver the callback isn't called, while with the GResolver the
> > operation is cancelled but the callback is still called ?
> 
> Yes. My understanding is that when the operation is cancelled, the callback
> returns with no entries and an error set along the lines of the operation
> having been cancelled. Because of that, it's caught in the normal error
> handling code.

So cancelling in the GAsync world means finish the operation asap. It can be the case, though unlikely, that after cancelling you still get a valid result at the end of the operation. 
Comment 6 Mike Ruprecht 2009-12-02 19:57:15 UTC
Updated the branch per your review. I did find and fix a case where cancelling it would crash it. Thanks for pointing that out.
Comment 7 Sjoerd Simons 2009-12-07 07:37:01 UTC
Merged, thanks!

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.