Bug 23920

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

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;
+}

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.

0bf47895312850b1a3434cae87e5c4a1d564108e:

You should still use g_hostname_to_ascii but i would just call it unconditionally.

(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. 

Updated the branch per your review. I did find and fix a case where cancelling it would crash it. Thanks for pointing that out.

Merged, thanks!