BIND 10 #183: UNIX domain socket for msgq

[BIND 10 Development]

#183: UNIX domain socket for msgq
-----------------------------+----------------------------------------------
 Reporter:  larissas         |        Owner:  jinmei                     
     Type:  defect           |       Status:  reviewing                  
 Priority:  major            |    Milestone:  04. 2nd Incremental Release
Component:  message-library  |   Resolution:                             
 Keywords:                   |    Sensitive:  0                          
-----------------------------+----------------------------------------------

Comment(by jinmei):

 Replying to [comment:10 jelte]:

 > And the two other things;

 > - SIN/SUN_LEN, I could add it, i was looking at configure and i think
 the way it is in trunk doesn't work (is SIN_LEN defined on your system?
 configure checks for another field than the one used in that ifdef
 statement...), if it is needed i can make a check for .sun_len and set it

 I've looked at configure.ac.  I think what we should do is to just test
 the (non)existence of sa_len and apply the result regardless of the
 specific AF (AF_INET, AF_INET6, AF_UNIX, or even AF_INET8).  We could
 borrow the check of BIND 9, for example:

 {{{
 AC_MSG_CHECKING(for sa_len in struct sockaddr)
 AC_TRY_COMPILE([
 #include <sys/types.h>
 #include <sys/socket.h>],
 [struct sockaddr sa; sa.sa_len = 0; return (0);],
         [AC_MSG_RESULT(yes)
         ISC_PLATFORM_HAVESALEN="#define ISC_PLATFORM_HAVESALEN 1"
         LWRES_PLATFORM_HAVESALEN="#define LWRES_PLATFORM_HAVESALEN 1"],
         [AC_MSG_RESULT(no)
         ISC_PLATFORM_HAVESALEN="#undef ISC_PLATFORM_HAVESALEN"
         LWRES_PLATFORM_HAVESALEN="#undef LWRES_PLATFORM_HAVESALEN"])
 AC_SUBST(ISC_PLATFORM_HAVESALEN)
 }}}

 In fact, the kernel assume the same organization for the first several
 fields of sockaddr_xxx, so it should be safe.

 (but if we complete the migration to ASIO, we won't have to be bothered
 with such a low level portability issues anyway...)

 > - Do you propose to not make a default value and explicitly use
 establish(NULL) in auth? (well actually, we should probably add a -m
 <path> option to auth in the first place. I left in the default value so
 as not to break compatibility, and changing the interface was hard enough
 as it was, with boost.asio and the pimpl idiom there :p

 I'm not sure how pimpl is related to this issue, but anyway, after looking
 at the code more closely, I've noticed we can (and IMO should) cnetralize
 configuration of socket_file in Session::establish(), not in the derived
 implementation classes.  Right each derived class has now the following
 duplicate code fragment:

 {{{
     if (socket_file != NULL) {
         socket_file = getenv("BIND10_MSGQ_SOCKET_FILE");
     }
     if (socket_file != NULL) {
         socket_file = BIND10_MSGQ_SOCKET_FILE;
     }
 }}}

 This is a common pattern where we need refactoring.

 As an additional bonus, by doing so we can ensure that we pass non NULL
 constant string to derived implementation classes, so we can use a const
 reference (then the reader don't have to worry about the possible case
 it's accidentally null).

 I'm attaching a suggested patch.

-- 
Ticket URL: <https://bind10.isc.org/ticket/183#comment:13>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development