Bug 47072

Initial branch with the following features

- Detect libdns_sd library and build system changes.

- bonjour-discovery-client and bonjor-self

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

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

and Other tweaks and fixes to make it work

todo:

* Verify connection before signaling we are connected (discovery-client)
* bonjour-contact-manager and bonjour-contact
* and what every it takes to make it all work.

(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.

(In reply to comment #2)
> 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?
> 

FWIW the libpurple Win32 prpl (using Bonjour) uses a g_get_host_name() wrapper where the avahi prpl uses avahi_client_get_host_name().

I think that's the best we can reasonably achieve as well.

path is now updated.

Thanks for fixing the issues. Here's a few further small things:

>_bonjour_socket_process_cb (GIOChannel *source,
>                             GIOCondition condition,

indent

>  /* Service is not announced yet */
>  if (TXTRecordGetLength (&priv->txt_record_presence) <= 0)
>    {
>      return TRUE;
>    }

The Avahi backend calls salut_avahi_self_set_caps from self_announce to recuperate this. You don't. I think you should add a call to _set_caps before DNSServiceRegister() in your self_announce, otherwise you won't have initial capabilities advertised (or then this is redundant in the Avahi backend too - but that could be fairly obscure to tell as it might have to do with races with when UpdateCapabilities is called etc though).

>  if (self->status_message != NULL)
>    {
>      error_type = TXTRecordSetValue (&priv->txt_record_presence, "msg",
>              strlen (self->status_message), self->status_message);
>      RETURN_ERROR_IF_FAIL (error_type, error);
>    }

This is all good, but if self->status_message IS NULL (the user has unset the status message), you must remove it from the record. TXTRecordRemoveValue() should do the trick.

additionally, you've rebased this quite weirdly, starting from this commit:

author	Olli Salli <olli.salli@collabora.co.uk>	2012-02-21 18:21:47 (GMT)
committer	 Siraj Razick <siraj.razick@collabora.co.uk>	2012-03-12 04:40:45 (GMT)
commit	457014470cbc07fa608b6556b3eea73833b74273 (patch) (side-by-side diff)
tree	169903d62d0b75fd90ffcd52044c285cc66a601f
parent	2afed9602d88fda812fb3a0d3e4678039cd3862b (diff)
download	telepathy-salut-457014470cbc07fa608b6556b3eea73833b74273.tar.gz
telepathy-salut-457014470cbc07fa608b6556b3eea73833b74273.tar.bz2
Update NEWS

That is, you've rebased a lot of patches already in master on top of some ... older ... branch.

To make this mergeable to current actual upstream master, please rebase on it. You need to use rebase -i I think, because git might no longer recognize that the earlier patches in your branch (like the above) are already in master. So update master and then git rebase -i master in your branch, and remove the lines for the patches that don't belong to your branch.

I imagine your windows build will break from Trever's FT code merged there - please fix that. I think it'll be as simple as conditionally linking to the GIO Windows lib instead of the UNIX one on Win32, and #ifdefing out UNIX socket code in ft-channel.c like you did in Gabble. The TCP socket code should, however, be used on windows.

While at that, you should update the master copy in your personal repo - it helps reviewers notice what's actually part of a still-unmerged branch and what's not, and what master state the branch has been based on (should be the latest).

That's about it from me. Jonny?

Okay I had a look at this. It generally looks fine. A couple of minor points here. I'll only insist that the ones which break the build are fixed, the rest are up to you.

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

You can use g_value_dup_object here if you want.

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

Indentation problem here.

+ DEBUG("Browser removed for %s", name);

Missing space.

+ if (mode)
+   {
+     error_type = DNSServiceUpdateRecord (priv->bonjour_service,
+         *dns_record, 0, size, data, time_out);
+
+     if (error_type != kDNSServiceErr_NoError)
+       {
+         DEBUG ("Error Updating Record : (%d)", error_type);
+         return error_type;
+       }
+   }
+ else
+   {
+     error_type = DNSServiceAddRecord (priv->bonjour_service,
+         dns_record, 0, rrtype, size, data, time_out);
+
+     if (error_type != kDNSServiceErr_NoError)
+       {
+         DEBUG ("Error Adding Record : (%d)", error_type);
+         return error_type;
+       }
+   }
+
+ return error_type;

You could probably refactor this to remove most of the duplication here.

+ /* Tubes are not currently supported by bonjour backend */
+#ifdef USE_BACKEND_AVAHI

I wonder if it would better to use:

    #ifndef USE_BACKEND_BONJOUR ?

(In reply to comment #5)
> additionally, you've rebased this quite weirdly, starting from this commit:

Yes this must be fixed before merging.

Now please please please run `make distcheck` and make sure it works, and report back here.

(In reply to comment #6)
> Okay I had a look at this. It generally looks fine. A couple of minor points
> here. I'll only insist that the ones which break the build are fixed, the rest
> are up to you.
> 
Thank You very much for looking into it :).


Ok the patch is now updated with the changes suggested by comment #6 and #7. (Including the improvements
suggested by Jonny).


I ran distcheck twice after updating the patches it passed fine.

and when rebaseing I had to add two more new patches. I didn't fix the functionality for windows there
just the build / compile issues, Since I'm busy with the contactmanager/contact for Bonjour backend.


> + case PROP_DISCOVERY_CLIENT:
> +   priv->discovery_client = g_value_get_object (value);
> +   g_object_ref (priv->discovery_client);
> 
> You can use g_value_dup_object here if you want.
> 
> + else if (!salut_bonjour_contact_has_services (SALUT_BONJOUR_CONTACT
> (contact)))
> +   {
> +    g_object_ref (contact);
> +   }
> 
> Indentation problem here.
> 
> + DEBUG("Browser removed for %s", name);
> 
> Missing space.
> 
> + if (mode)
> +   {
> +     error_type = DNSServiceUpdateRecord (priv->bonjour_service,
> +         *dns_record, 0, size, data, time_out);
> +
> +     if (error_type != kDNSServiceErr_NoError)
> +       {
> +         DEBUG ("Error Updating Record : (%d)", error_type);
> +         return error_type;
> +       }
> +   }
> + else
> +   {
> +     error_type = DNSServiceAddRecord (priv->bonjour_service,
> +         dns_record, 0, rrtype, size, data, time_out);
> +
> +     if (error_type != kDNSServiceErr_NoError)
> +       {
> +         DEBUG ("Error Adding Record : (%d)", error_type);
> +         return error_type;
> +       }
> +   }
> +
> + return error_type;
> 
> You could probably refactor this to remove most of the duplication here.
> 
> + /* Tubes are not currently supported by bonjour backend */
> +#ifdef USE_BACKEND_AVAHI
> 
> I wonder if it would better to use:
> 
>     #ifndef USE_BACKEND_BONJOUR ?
> 
> (In reply to comment #5)
> > additionally, you've rebased this quite weirdly, starting from this commit:
> 
> Yes this must be fixed before merging.
> 
> Now please please please run `make distcheck` and make sure it works, and
> report back here.

Thanks Jonny for the double-check!

(In reply to comment #7)
> and when rebaseing I had to add two more new patches. I didn't fix the
> functionality for windows there
> just the build / compile issues, Since I'm busy with the contactmanager/contact
> for Bonjour backend.
> 

Yeah, that's sensible - I've created the new bug 47272 for this.

I've verified that all the other issues spotted by Jonny and me along the way are now fixed, and the branch is now correctly based on current master, so I merged it. Salut now has support to appear on the network on Windows. Onward to the remaining part of the backend - discovering others.