[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 #7 from Siraj Razick <siraj.razick at collabora.co.uk> 2012-03-22 17:40:22 PDT ---
(In reply to comment #6)
> 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?

me too, I just followed whats in avahi-contact-manager so it follows a similar
logic. how do you propose I change this ?

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