[Bug 47429] Support SalutContact and SautContact-manager with Bonjour backend

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=47429

Jonny Lamb <jonny.lamb at collabora.co.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Status Whiteboard|                            |review-

--- Comment #3 from Jonny Lamb <jonny.lamb at collabora.co.uk> 2012-03-22 12:44:38 UTC ---
> We need to pass published_name at host when registering for the service current
> code passes NULL, which lets mdns choose it's name.

OOI what does mdns choose if you pass NULL?

> + priv->svc_ref_table = NULL;
> + priv->svc_source_table = NULL;

Huh? These are set to NULL in _init and set to real values in _watch_svc_ref.
If _watch_svc_ref is never called then g_hash_table_foreach_remove,
g_hash_table_remove_all, and g_hash_table_destroy will be all called on NULL.
Creating two hash tables in the discovery client isn't so costly; just create
them in _init then you can remove the checks in _watch_svc_ref and
_drop_svc_ref and not assert in said case.

> + g_hash_table_remove (priv->svc_source_table, channel);
> +
> + return TRUE;

You're calling g_hash_table_foreach_remove which takes a GHRFunc which is
defined to  return "TRUE if the key/value pair should be removed from the
GHashTable." However, just before returning TRUE you remove the key/value pair
from the table anyway..?

In fact, why don't you just make this the value destroy function of the
svc_source_table GHashTable? It would simplify this code a lot.

> + priv->svc_source_table = g_hash_table_new_full (g_direct_hash,
> +     g_direct_equal,NULL, NULL);

Missing space.

> + priv->discovery_client = g_value_get_object (value);
> + g_object_ref (priv->discovery_client);

g_value_dup_object

> + socket_address = g_inet_socket_address_new (addr, port);
> + g_object_ref (addr);

Why do you suddenly ref addr? The equivalent code in SalutAvahiContact which
you've otherwise used as a base does this:

    socket_address = g_inet_socket_address_new (addr, port);
    g_object_unref (addr);

so I'm confused?

> +     g_array_append_val (addresses, s_address);
> +    }
> +}

Indentation.

> + addresses =
> +   g_array_sized_new (TRUE, TRUE, sizeof (salut_contact_address_t), 0);

It seems odd to use sized_new when you just use 0 and you /could/ find out the
exact size if you wanted? Given finding the size is O(n) perhaps it's not worth
it though; what do you think? If you used GQueue you could just use .length
which would be even nicer.

Every time a GQueue is used I cheer!

> + resolvers_left = g_slist_length (priv->resolvers);
> +
> + g_free (ctx);
> +
> + if (resolvers_left == 0)
> +   salut_contact_lost (SALUT_CONTACT (self));

g_[s]list_length(list) == 0 if and only if list == NULL...

> + ctx = g_new0 (SalutBonjourResolveCtx, 1);

Slice allocating this stuff is preferred these days; use g_slice_new0 and
g_slice_free.

> + priv->resolvers = g_slist_prepend (priv->resolvers, ctx);

I really hate this "prepend to a list because it's quicker than appending"
thing. Yes, turn priv->resolvers into a GQueue.


> + if (error_type != kDNSServiceErr_NoError)
> +   {
> +     DEBUG ("Resolver failed witih : (%d)", error_type);
> +     return ;
> +   }

*with

There's an extra space after "return".

Should we not still thaw the salut contact here?

+ if (error_type !=kDNSServiceErr_NoError)

Missing space.

> + else if (!salut_bonjour_contact_has_services (SALUT_BONJOUR_CONTACT
> +     (contact)))
> +   {
> +     g_object_ref (contact);
> +   }

but later on:

+ if (salut_bonjour_contact_has_services (SALUT_BONJOUR_CONTACT (contact)))
+   {
+     /* We reffed this contact as it has services */
+     g_object_unref (contact);
+   }

So this reffing with services confuses me?

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA Contact for the bug.