Thread: Patch: update Bonjour support to the newer non-deprecated API

Patch: update Bonjour support to the newer non-deprecated API

From
Tom Lane
Date:
I've grown a bit tired of reading the "'DNSServiceRegistrationCreate' is
deprecated" warnings in OS X builds, and I'm also wondering whether any
of the recently reported problems with Snow Leopard might trace to our
use of an API that Apple has been deprecating since 2003.  Hence,
attached is a patch that replaces our use of the DNSServiceDiscovery.h
API with the more modern dns_sd.h API.  As-is, this would break Bonjour
functionality in OS X 10.2 and before, but I really doubt that anyone
still cares about that --- any objections out there?

Like the original coding, this will bind the Bonjour service name on
all available interfaces.  I suspect that we ought to do something
different if we have been told to bind to a subset of interfaces,
but it's not entirely clear to me how to get the appropriate "interface
indexes" given the information we have (particularly, the listening
socket FDs).  In any case, fixing that seems like added functionality.

In principle this might enable use of Bonjour on non-Apple OSes, but
I'm not personally interested enough to test that ...

Comments, objections?

            regards, tom lane

Index: configure.in
===================================================================
RCS file: /cvsroot/pgsql/configure.in,v
retrieving revision 1.609
diff -c -r1.609 configure.in
*** configure.in    26 Aug 2009 22:24:42 -0000    1.609
--- configure.in    7 Sep 2009 03:00:24 -0000
***************
*** 1066,1072 ****
  fi

  if test "$with_bonjour" = yes ; then
!   AC_CHECK_HEADER(DNSServiceDiscovery/DNSServiceDiscovery.h, [], [AC_MSG_ERROR([header file
<DNSServiceDiscovery/DNSServiceDiscovery.h>is required for Bonjour])]) 
  fi

  # for contrib/uuid-ossp
--- 1066,1072 ----
  fi

  if test "$with_bonjour" = yes ; then
!   AC_CHECK_HEADER(dns_sd.h, [], [AC_MSG_ERROR([header file <dns_sd.h> is required for Bonjour])])
  fi

  # for contrib/uuid-ossp
Index: src/backend/postmaster/postmaster.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/postmaster/postmaster.c,v
retrieving revision 1.594
diff -c -r1.594 postmaster.c
*** src/backend/postmaster/postmaster.c    31 Aug 2009 19:41:00 -0000    1.594
--- src/backend/postmaster/postmaster.c    7 Sep 2009 03:00:24 -0000
***************
*** 89,95 ****
  #endif

  #ifdef USE_BONJOUR
! #include <DNSServiceDiscovery/DNSServiceDiscovery.h>
  #endif

  #include "access/transam.h"
--- 89,95 ----
  #endif

  #ifdef USE_BONJOUR
! #include <dns_sd.h>
  #endif

  #include "access/transam.h"
***************
*** 309,324 ****
  extern int    optreset;            /* might not be declared by system headers */
  #endif

  /*
   * postmaster.c - function prototypes
   */
  static void getInstallationPaths(const char *argv0);
  static void checkDataDir(void);
-
- #ifdef USE_BONJOUR
- static void reg_reply(DNSServiceRegistrationReplyErrorType errorCode,
-           void *context);
- #endif
  static void pmdaemonize(void);
  static Port *ConnCreate(int serverFd);
  static void ConnFree(Port *port);
--- 309,323 ----
  extern int    optreset;            /* might not be declared by system headers */
  #endif

+ #ifdef USE_BONJOUR
+ static DNSServiceRef bonjour_sdref = NULL;
+ #endif
+
  /*
   * postmaster.c - function prototypes
   */
  static void getInstallationPaths(const char *argv0);
  static void checkDataDir(void);
  static void pmdaemonize(void);
  static Port *ConnCreate(int serverFd);
  static void ConnFree(Port *port);
***************
*** 855,869 ****

  #ifdef USE_BONJOUR
      /* Register for Bonjour only if we opened TCP socket(s) */
!     if (ListenSocket[0] != -1 && bonjour_name != NULL)
      {
!         DNSServiceRegistrationCreate(bonjour_name,
!                                      "_postgresql._tcp.",
!                                      "",
!                                      htons(PostPortNumber),
!                                      "",
!                                      (DNSServiceRegistrationReply) reg_reply,
!                                      NULL);
      }
  #endif

--- 854,891 ----

  #ifdef USE_BONJOUR
      /* Register for Bonjour only if we opened TCP socket(s) */
!     if (ListenSocket[0] != -1)
      {
!         DNSServiceErrorType err;
!
!         /*
!          * We pass 0 for interface_index, which will result in registering
!          * for all available interfaces.  It's not entirely clear from the
!          * DNS-SD docs whether this would be appropriate if we have bound
!          * to just a subset of the available interfaces.
!          */
!         err = DNSServiceRegister(&bonjour_sdref,
!                                  0,
!                                  0,
!                                  bonjour_name,
!                                  "_postgresql._tcp.",
!                                  NULL,
!                                  NULL,
!                                  htons(PostPortNumber),
!                                  0,
!                                  NULL,
!                                  NULL,
!                                  NULL);
!         if (err != kDNSServiceErr_NoError)
!             elog(LOG, "DNSServiceRegister() failed: error code %ld",
!                  (long) err);
!         /*
!          * We don't bother to read the mDNS daemon's reply, and we expect
!          * that it will automatically terminate our registration when the
!          * socket is closed at postmaster termination.  So there's nothing
!          * more to be done here.  However, the bonjour_sdref is kept around
!          * so that forked children can close their copies of the socket.
!          */
      }
  #endif

***************
*** 1192,1209 ****
  }


- #ifdef USE_BONJOUR
-
- /*
-  * empty callback function for DNSServiceRegistrationCreate()
-  */
- static void
- reg_reply(DNSServiceRegistrationReplyErrorType errorCode, void *context)
- {
- }
- #endif   /* USE_BONJOUR */
-
-
  /*
   * Fork away from the controlling terminal (silent_mode option)
   *
--- 1214,1219 ----
***************
*** 2004,2009 ****
--- 2014,2025 ----
          syslogPipe[0] = 0;
  #endif
      }
+
+ #ifdef USE_BONJOUR
+     /* If using Bonjour, close the connection to the mDNS daemon */
+     if (bonjour_sdref)
+         close(DNSServiceRefSockFD(bonjour_sdref));
+ #endif
  }



Re: Patch: update Bonjour support to the newer non-deprecated API

From
"David E. Wheeler"
Date:
On Sep 6, 2009, at 8:17 PM, Tom Lane wrote:

> In principle this might enable use of Bonjour on non-Apple OSes, but
> I'm not personally interested enough to test that ...
>
> Comments, objections?

+1 Seems like a no-brainer.

Best,

David


Re: Patch: update Bonjour support to the newer non-deprecated API

From
Alvaro Herrera
Date:
Tom Lane wrote:

> In principle this might enable use of Bonjour on non-Apple OSes, but
> I'm not personally interested enough to test that ...

With this patch and Avahi's compatibility headers I can successfully
compile --enable-bonjour on my Debian system.  I haven't tested it.

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support


Re: Patch: update Bonjour support to the newer non-deprecated API

From
Alvaro Herrera
Date:
Alvaro Herrera wrote:
> Tom Lane wrote:
> 
> > In principle this might enable use of Bonjour on non-Apple OSes, but
> > I'm not personally interested enough to test that ...
> 
> With this patch and Avahi's compatibility headers I can successfully
> compile --enable-bonjour on my Debian system.  I haven't tested it.

Hmm, but it doesn't link unless I manually add -ldns_sd to the link
command.

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support


Re: Patch: update Bonjour support to the newer non-deprecated API

From
Alvaro Herrera
Date:
Alvaro Herrera wrote:

> Hmm, but it doesn't link unless I manually add -ldns_sd to the link
> command.

And then it throws a warning on start, and fails to work anyway:

*** WARNING *** The program 'postgres' uses the Apple Bonjour compatibility layer of Avahi.
*** WARNING *** Please fix your application to use the native API of Avahi!
*** WARNING *** For more information see <http://0pointer.de/avahi-compat?s=libdns_sd&e=postgres>
LOG:  DNSServiceRegister() failed: error code -65540

So we're not better than when we started, but it doesn't cause any
problem if not enabled.

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.


Re: Patch: update Bonjour support to the newer non-deprecated API

From
Tom Lane
Date:
Alvaro Herrera <alvherre@commandprompt.com> writes:
> *** WARNING *** The program 'postgres' uses the Apple Bonjour compatibility layer of Avahi.
> *** WARNING *** Please fix your application to use the native API of Avahi!
> *** WARNING *** For more information see <http://0pointer.de/avahi-compat?s=libdns_sd&e=postgres>
> LOG:  DNSServiceRegister() failed: error code -65540

Hmm, I read in their documentation that the dns_sd.h interface was
deprecated, but not that it had been intentionally disabled.
Seems like they want to drive users away rather than attract them.

> So we're not better than when we started, but it doesn't cause any
> problem if not enabled.

The patch as I gave it intentionally didn't change any user-visible
behavior, but one thing that is bothering me is that if USE_BONJOUR
is selected, the postmaster will *always* try to advertise itself
via DNS-SD.  There's no provision for enabling the feature or not
at run time, which is a bad thing for packagers: they have to decide
for their users whether to turn it on.  There was discussion in
connection with the Avahi patch last year to the effect that some
people thought advertising the postmaster might be a security issue
for them.  So I'm thinking we ought to fix that while we're messing
with it.

The two possibilities for that seem to be to change the meaning of
bonjour_name = '' (have it mean "no advertisement" instead of
"default to service name = computer's name"), or to add a separate
boolean GUC.  If the latter, is the default 'on' or 'off'?  Opinions?
        regards, tom lane


Re: Patch: update Bonjour support to the newer non-deprecated API

From
Alvaro Herrera
Date:
Tom Lane wrote:
> Alvaro Herrera <alvherre@commandprompt.com> writes:
> > *** WARNING *** The program 'postgres' uses the Apple Bonjour compatibility layer of Avahi.
> > *** WARNING *** Please fix your application to use the native API of Avahi!
> > *** WARNING *** For more information see <http://0pointer.de/avahi-compat?s=libdns_sd&e=postgres>
> > LOG:  DNSServiceRegister() failed: error code -65540
> 
> Hmm, I read in their documentation that the dns_sd.h interface was
> deprecated, but not that it had been intentionally disabled.
> Seems like they want to drive users away rather than attract them.

I think it is supposed to work; the code suggests that it should.  I
can't quite find out what the error number is supposed to mean though.
The source is here:
http://avahi.sourcearchive.com/documentation/0.6.25-1ubuntu1/avahi-compat-libdns__sd_2compat_8c-source.html

... ah!  here it is -- BadParam:
http://avahi.sourcearchive.com/documentation/0.6.25-1ubuntu1/dns__sd_8h-source.html


> The patch as I gave it intentionally didn't change any user-visible
> behavior, but one thing that is bothering me is that if USE_BONJOUR
> is selected, the postmaster will *always* try to advertise itself
> via DNS-SD.  There's no provision for enabling the feature or not
> at run time, which is a bad thing for packagers: they have to decide
> for their users whether to turn it on.  There was discussion in
> connection with the Avahi patch last year to the effect that some
> people thought advertising the postmaster might be a security issue
> for them.  So I'm thinking we ought to fix that while we're messing
> with it.
> 
> The two possibilities for that seem to be to change the meaning of
> bonjour_name = '' (have it mean "no advertisement" instead of
> "default to service name = computer's name"), or to add a separate
> boolean GUC.  If the latter, is the default 'on' or 'off'?  Opinions?

I have a mild preference towards having a new GUC to shut it off
explicitely; and the default should be off to avoid the possible
security hole (equivalent to having listen_addresses default to
localhost, I think.  On the other hand, if listen_addresses is set to
that, there is no security hole.  I assume we're only publishing on
addresses we're listening on, not all addresses?)

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support


Re: Patch: update Bonjour support to the newer non-deprecated API

From
David Fetter
Date:
On Mon, Sep 07, 2009 at 12:50:37PM -0400, Tom Lane wrote:
> 
> The two possibilities for that seem to be to change the meaning of
> bonjour_name = '' (have it mean "no advertisement" instead of
> "default to service name = computer's name"), or to add a separate
> boolean GUC.  If the latter, is the default 'on' or 'off'?
> Opinions?

+1 for separate boolean GUC, default off.  I know extra GUCs aren't
great, but I don't see another way :/

Cheers,
David.
-- 
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter      XMPP: david.fetter@gmail.com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate


Re: Patch: update Bonjour support to the newer non-deprecated API

From
Tom Lane
Date:
David Fetter <david@fetter.org> writes:
> On Mon, Sep 07, 2009 at 12:50:37PM -0400, Tom Lane wrote:
>> The two possibilities for that seem to be to change the meaning of
>> bonjour_name = '' (have it mean "no advertisement" instead of
>> "default to service name = computer's name"), or to add a separate
>> boolean GUC.  If the latter, is the default 'on' or 'off'?
>> Opinions?

> +1 for separate boolean GUC, default off.  I know extra GUCs aren't
> great, but I don't see another way :/

Yeah, the default service name seems like a potentially useful behavior
in some contexts, so taking that behavior away doesn't seem nice.
Separate bool it is...
        regards, tom lane


Re: Patch: update Bonjour support to the newer non-deprecated API

From
Tom Lane
Date:
Alvaro Herrera <alvherre@commandprompt.com> writes:
> Tom Lane wrote:
>> Hmm, I read in their documentation that the dns_sd.h interface was
>> deprecated, but not that it had been intentionally disabled.

> I think it is supposed to work; the code suggests that it should.  I
> can't quite find out what the error number is supposed to mean though.
> The source is here:
> http://avahi.sourcearchive.com/documentation/0.6.25-1ubuntu1/avahi-compat-libdns__sd_2compat_8c-source.html

> ... ah!  here it is -- BadParam:
> http://avahi.sourcearchive.com/documentation/0.6.25-1ubuntu1/dns__sd_8h-source.html

Interesting.  I coded the call to default everything it could, but maybe
Avahi is a little less forgiving about NULL parameters.  In particular
it wouldn't surprise me if they thought that there *must* be a callback
function --- we're playing fast and loose with the API by not bothering
to check for a response from the daemon.
        regards, tom lane


Re: Patch: update Bonjour support to the newer non-deprecated API

From
Tom Lane
Date:
Alvaro Herrera <alvherre@commandprompt.com> writes:
> The source is here:
> http://avahi.sourcearchive.com/documentation/0.6.25-1ubuntu1/avahi-compat-libdns__sd_2compat_8c-source.html

After reading that code, I am content to remain forcibly non compatible
with it.  If we relied on this it would inject threading into the
postmaster, which was precisely the thing that got the previous Avahi
patch rejected last year :-(

Apple's implementation of the same API does not appear to require
any threading:
http://www.opensource.apple.com/source/mDNSResponder/mDNSResponder-176.3/mDNSShared/dnssd_clientstub.c
Hm, seems to be Apache license ...
        regards, tom lane


Re: Patch: update Bonjour support to the newer non-deprecated API

From
Alvaro Herrera
Date:
Tom Lane wrote:
> Alvaro Herrera <alvherre@commandprompt.com> writes:
> > The source is here:
> > http://avahi.sourcearchive.com/documentation/0.6.25-1ubuntu1/avahi-compat-libdns__sd_2compat_8c-source.html
> 
> After reading that code, I am content to remain forcibly non compatible
> with it.  If we relied on this it would inject threading into the
> postmaster, which was precisely the thing that got the previous Avahi
> patch rejected last year :-(

Should we inject some sort of compile-time rejection of the
compatibility API?  Like, say

#ifdef AVAHI_SERVICE_COOKIE
#error The Avahi implementation is incompatible
#endif

inside some #ifdef BONJOUR block?

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.


Re: Patch: update Bonjour support to the newer non-deprecated API

From
Tom Lane
Date:
I wrote:
> I've grown a bit tired of reading the "'DNSServiceRegistrationCreate' is
> deprecated" warnings in OS X builds, and I'm also wondering whether any
> of the recently reported problems with Snow Leopard might trace to our
> use of an API that Apple has been deprecating since 2003.  Hence,
> attached is a patch that replaces our use of the DNSServiceDiscovery.h
> API with the more modern dns_sd.h API.

BTW, I cannot find any evidence that our old Bonjour code doesn't work
on Snow Leopard: it compiles, with just the same deprecation warning
as before, and the postmaster runs, and you can see the Bonjour
registration appear with something like 'dns-sd -B _postgresql' ---
and disappear again when the postmaster exits.  I had guessed that
Jerry LeVan's DNS issues might be somehow related to this code, but
apparently not.

So I'm inclined to change the code now that we've got a replacement
that doesn't cause the deprecation warning, but there doesn't seem to
be any need to back-patch it.

I will also see about adding a separate boolean control GUC, as
discussed.
        regards, tom lane


Re: Patch: update Bonjour support to the newer non-deprecated API

From
Tom Lane
Date:
Alvaro Herrera <alvherre@commandprompt.com> writes:
> Should we inject some sort of compile-time rejection of the
> compatibility API?  Like, say

> #ifdef AVAHI_SERVICE_COOKIE
> #error The Avahi implementation is incompatible
> #endif

> inside some #ifdef BONJOUR block?

I looked into this and couldn't find any obvious candidate symbol to
test for.  I'm not that excited about throwing roadblocks in the way
of somebody who wants to try it, anyway --- my feeling is more like
"if it breaks you get to keep both pieces".
        regards, tom lane


Re: Patch: update Bonjour support to the newer non-deprecated API

From
Alvaro Herrera
Date:
Tom Lane wrote:
> Alvaro Herrera <alvherre@commandprompt.com> writes:
> > Should we inject some sort of compile-time rejection of the
> > compatibility API?  Like, say
> 
> > #ifdef AVAHI_SERVICE_COOKIE
> > #error The Avahi implementation is incompatible
> > #endif
> 
> > inside some #ifdef BONJOUR block?
> 
> I looked into this and couldn't find any obvious candidate symbol to
> test for.  I'm not that excited about throwing roadblocks in the way
> of somebody who wants to try it, anyway --- my feeling is more like
> "if it breaks you get to keep both pieces".

Agreed -- that seems reasonable considering that one needs to
explicitely pass --enable-bonjour anyway.

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.