[Bug 47072] Add self-advertise support on Windows using the Apple Bonjour DNS-SD implementation

[bugzilla-daemon at freedesktop.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|Add support for Apple       |Add self-advertise support
                   |Bonjour backend on windows  |on Windows using the Apple
                   |                            |Bonjour DNS-SD
                   |                            |implementation

--- Comment #2 from Olli Salli <olli.salli at collabora.co.uk> 2012-03-08 15:09:21 PST ---
(In reply to comment #1)
> Initial branch with the following features
> 
> - Detect libdns_sd library and build system changes.

+  AC_HELP_STRING([--with-backend=[avahi/bonjour/no]],
                  [Zeroconf backend to use]),
   [], [with_backend=avahi])

Could make the default be bonjour on Windows. Avahi will never work.

Actually, please further make this --with-backend=bonjour/no on Windows
entirely. avahi/bonjour/no makes sense for the "Other OS case" (as it's not
really just Linux) if somebody ports Salut to OS X one day.

+    $(CORE_SOURCES) $(SIGNAL_MARSHAL_SOURCES) ) \

Good call to add a nice variable for the rather complicatedly determined signal
marshaller sources. However, your variable already includes CORE_SOURCES so
mentioned it again is redundant.

+  AM_CFLAGS += -DNOT_HAVE_GETOPT \

Looking at
http://www.opensource.apple.com/source/mDNSResponder/mDNSResponder-333.10/mDNSShared/dns_sd.h
- it doesn't respond in any way to NOT_HAVE_GETOPT. So please omit this weird
non-namespaced define.

The build system changes look OK otherwise. For the benefit of other reviewers,
it should be noted that we needed to modify the dns-sd client library otherwise
as well, so we added a pkgconfig file to our modified version for easily using
it as a dep here.

> 
> - bonjour-discovery-client and bonjor-self

BonjourDiscoveryClient:

+  g_signal_emit (G_OBJECT (self), signals[STATE_CHANGED], 0,
+      SALUT_DISCOVERY_CLIENT_STATE_CONNECTED);

Use change_state() to do this, otherwise you'll leave priv->state (and the
"state" property consequently) still DISCONNECTED

+salut_bonjour_discovery_client_get_host_name_fqdn (SalutDiscoveryClient *clt)

you never assign anything to priv->fqdn? So it'll be NULL and this'll always
return NULL. The Avahi daemon provides this information to clients, but... I
don't see anything similar in the Bonjour APIs. Luckily, the only thing where
this is used is when creating the SI bytestream manager. And that in turn is
only used by stream tubes, which we don't need for this port.

I advise dropping the priv->fqdn variables etc, and making get_host_name_fqdn
*warn* that the bonjour backend doesn't support that and return NULL explicitly
for now.

+salut_bonjour_discovery_client_create_self (SalutDiscoveryClient *client,
+                                           SalutConnection *connection,

Indent mismatch

+  gpointer bonjour_client;

Some copypasta from the avahi backend, unused? Please remove.

--

BonjourSelf:

+   char *host_name = (char *) malloc (kDNSServiceMaxDomainName);
+       error_type = DNSServiceConstructFullName (host_name, name, regtype,
domain);
+       _self->name = g_strdup_printf ("%s@%s", _self->published_name,
host_name);

Mmm... I don't think concatenating those things result in a sensible host name
(especially one unique on your network). Won't this lead to a very weird and
potentially conflicting JID? (And possibly make your address unresolvable in
the network...)

The Avahi backend uses avahi_client_get_host_name(). Which queries the daemon
for the host name it chose. Basically that takes the one in the config file, or
if missing, what gethostname() returns, and increments a digit in the end until
nobody else in the network has the same name. Apple mDNSResponder seems to do
the same, but doesn't seem to have any way to report back what it ended up
choosing. But don't we need that knowledge here?

Maybe I'm just too tired for this at the moment... what do your JIDs look like?

mDNS experts - what should this stuff be formed of?

> 
> - dummy contact-manager (untill we add full support in stage two)

It's a bit weird mixture of commented out avahi code and all now. OK for now
but to make reviewing the actual implementation easier, in the branch for that
please first have one commit which eradicates all weird dead code from it.
Otherwise the patches will look like

- removed
- unrelated
- crap
+ some
+ actual
+ bonjour
+ code
<empty line woohoo it's the same in both!!>
- more
- weird
- cruft

> 
> what works :
>    - getting the account online on windows
>    - setting presence, caps, alias and avatar

FYI siraj's been manually testing this stuff out with mc-tool & poking D-Bus
methods and running some tp-glib examples against it, and verifying iChats and
linux-based saluts in the local network see it correctly.

> 
> todo:
> 
> * Verify connection before signaling we are connected (discovery-client)

I don't think we should try and set up a shared connection now - because the
dns-sd lib semantics for that are ridiculously complex (e.g. the MORE_COMING
flag being common to all operations sharing the connection... so it depends on
what other operations are running if you're going to get that when processing
the result for a particular one or not).

> * bonjour-contact-manager and bonjour-contact

Separate branch & bug for that please, this is enough to review in one go :)
I've altered this bug's description accordingly.

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