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

[bugzilla-daemon at freedesktop.org]

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

--- Comment #6 from Siraj Razick <siraj.razick at collabora.co.uk> 2012-03-22 17:29:44 PDT ---
Thanks for you the review
(In reply to comment #3)
> > 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?

just the host name. 

reset i'll fix.
> 
> > + 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.