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

[bugzilla-daemon at freedesktop.org]

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

Olli Salli <olli.salli at collabora.co.uk> changed:

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

--- Comment #1 from Olli Salli <olli.salli at collabora.co.uk> 2012-03-17 06:32:01 PDT ---
Thanks for having that initial commit which removes the unneeded avahi-specific
code, that makes the rest of the diffs much more readable.

Sharing the code for DNSServiceRef FD management is a good idea in principle;
however the implementation is currently rather broken.

+void
+salut_bonjour_cleanup_socket (DNSServiceRef *service)
+{
+  GIOChannel *channel = NULL;
+
+  channel = g_io_channel_win32_new_socket (
+     DNSServiceRefSockFD ((*service)));
+
+  g_io_channel_unref (channel);
+}

This is totally silly. You create a new GIOChannel and destroy it whereas I
think you'd rather want to clean up the old one. Currently it'll stay in
existence and still be registered as a watch.

To clean up the old one you need to look it up somehow. But you don't have any
way to do that. Alternatives for fixing that:

1) Return the GIOChannel pointer or a structure with that + the DNSServiceRef
from add_socket_watch and pass that to cleanup_socket instead of just the
ServiceRef
2) have a GHashTable from DNSServiceRef * to GIOChannel * which you insert to
in add_socket_watch

1 would be messy IMO, as then the point of this code that users don't have to
care about the GIOChannels would be lost.

2 is better; the current simplistic API would be all that's required. But where
to put that GHashTable? There is no object where you could put it as a member.
Global var would be messy and impossible to clean up. I think cleanest would be
to turn this code into BonjourDiscoveryClient member functions, as the users of
this functionality always have a ref to the disco client. This could be done as
follows:
* add_socket_watch becomes bonjour_discovery_client_watch_svc_ref
* cleanup_socket becomes bonjour_discovery_client_drop_svc_ref
* put a GHashTable DNSSR * -> GIOCh * in BonjourDiscoClientPriv
* clean up all the remaining stuff (and the hash table itself of course) on
dispose

The new names reflect what these functions do somewhat better.

With the service ref handling fixed, couldn't you also use it in bonjour-self
now? That also illuminated another issue in my mind: shouldn't we
DNSServiceRefDeallocate() the service we register in BonjourSelf when it is
disposed - otherwise we'll leak it, and leave the service registered on the
network? You can fix that in this branch.

In fact, is there a situation where we wouldn't want to dealloc service refs
when we remove them from the main loop? It's not very useful anymore at least
as new events on it are not processed. You could make drop_svc_ref also
deallocate the service ref so you don't forget to do that.

Now to the actual backend code...

BonjourContact:

This class needs to make a decision: does it support multiple services for a
single contact, or does it not? Everything has to work properly with multiple
services, or everything has to explicitly forbid multiple services.

The Salut Avahi backend supports multiple services per contact perfectly, by
the virtue of keeping per-service data in a "resolvers" linked list.

Judging by _mdns_service_browse_callback in
http://developer.pidgin.im/viewmtn/revision/file/3ecd9b9823c0c23f80314cbc8f9d07092cccd91e/libpurple/protocols/bonjour/mdns_win32.c,
the libpurple Bonjour win32 backend (using the same library as we are using)
also supports multiple services per contact ("buddy"), much in the same fashion
as our Avahi backend.

So I don't really see how we could justify omitting support for that -
especially in the current half-assed fashion of just being silently broken in
that case.

Let's now consider how SalutBonjourContact fares with multiple services for a
contact. There is a function called salut_bonjour_contact_add_service(), which
would indicate multiple services can be added to a single contact.

However, when called the second time for a contact priv->resolve_service will
be overwritten - leaving no reference to the previous service's one to stop it
later. Also, when a single service is removed with service_remove(), the whole
contact will be declared "lost".

SalutBonjourContact has a resolvers SList as well... but it contains...
addresses, responses for address resolving calls we make each time we receive a
new result for the contact service's TXT record. We'll receive those e.g. when
a contact changes their presence - while still having the same address for the
service. We'll just keep adding duplicate addresses there - what is the point
in that?

On the subject of addresses, you currently salut_contact_found() the contact at
the time the first TXT record is received - as I've said numerous times, that
is wrong. The contact is not contactable (has any addresses) until the first
addresses have been discovered through the GetAddrInfo operation.

Okay, enough pointing out what's wrong. Ideas for fixing this stuff follow.

* store full blown "resolver" objects in the resolvers list
* add one each time add_service is called
* remove the correct one (matching the name, type, domain tuple) when
remove_service is called

-> the resolver objects/structs have to contain at least copies of the name,
type and domain values, so we know which one is which

* each resolver should have a DNSServiceResolve job - that's its main purpose
* when we've received a result, there should be a GetAddrInfo job as well to
resolve the address

-> the struct should also contain DNSServiceRefs for a service resolve and
address resolve

* they should still contain the address info as well to be able to implement
get_addresses and has_address
* BUT ONLY ONE ADDRESS FOR EACH SERVICE

-> have a single address member, where you copy the service address when
GetAddrInfo produces a result

You should also do what AvahiContact does with _lost - declare a contact lost
when the last such resolver is removed.

So. As a conceptual recap:
* A contact can have multiple resolvers
* There is a resolver for each (name, type, domain) tuple
* Each resolver watches a TXT record and the SRV record
* And resolves the SRV record values to addresses
* A contact exists when it has parsed at least one service's TXT record and
resolved the address from the corresponding SRV record
* A contact ceases to exist when there are no longer any services from the TXT
records of which we would receive info for it and through the SRV records be
able to contact it

BonjourContactManager:

+  /* Filter out self contact, I don't feel good about, how I do it here.
+     so suggestions are welcome. And secondly g_get_host_name() return the
+     host name in upper case while ServiceName returned in lowercase.
+  */
+  if (g_ascii_strcasecmp (name, g_get_host_name ()) == 0)
+    return;
+

More importantly than case, AIUI the name is a JID not just a host name?
Compare against SalutSelf::name? If you populate that correctly in the
BonjourSelf subclass, it should be your JID. You might need to
normalize/lowercase it there. 

Actually when I look at BonjourSelf now - do you publish the record correctly
at all? Avahi uses self->published_name for naming the record, you don't. Have
you tested at all how the records appear on the network? You might end up
calling yourself "host" instead of "siraj at machine".

  if (!priv->all_for_now)
    {
      g_signal_emit_by_name (self, "all-for-now");
      priv->all_for_now = TRUE;
    }

Shouldn't you consider the MoreComing flag - if bonjour says there's more
coming immediately, clearly it's not all for now yet.

The multiple service handling in the ContactManager looks OK - but that doesn't
save Contact.

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