You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Ruediger Pluem <rp...@apache.org> on 2009/02/19 21:45:30 UTC
Re: svn commit: r745763 - in /apr/apr/trunk: ./ build/ include/ include/arch/unix/
network_io/unix/ test/
On 02/19/2009 08:15 AM, mturk@apache.org wrote:
> Author: mturk
> Date: Thu Feb 19 07:15:23 2009
> New Revision: 745763
>
> URL: http://svn.apache.org/viewvc?rev=745763&view=rev
> Log:
> Enable unix domain (AF_UNIX) sockets if supported by the OS
>
> Modified:
> apr/apr/trunk/CHANGES
> apr/apr/trunk/build/apr_network.m4
> apr/apr/trunk/configure.in
> apr/apr/trunk/include/apr.h.in
> apr/apr/trunk/include/apr.hnw
> apr/apr/trunk/include/apr.hw
> apr/apr/trunk/include/apr_network_io.h
> apr/apr/trunk/include/arch/unix/apr_arch_networkio.h
> apr/apr/trunk/network_io/unix/sockaddr.c
> apr/apr/trunk/network_io/unix/sockets.c
> apr/apr/trunk/network_io/unix/sockopt.c
> apr/apr/trunk/test/sockchild.c
> apr/apr/trunk/test/testsock.c
>
> Modified: apr/apr/trunk/network_io/unix/sockaddr.c
> URL: http://svn.apache.org/viewvc/apr/apr/trunk/network_io/unix/sockaddr.c?rev=745763&r1=745762&r2=745763&view=diff
> ==============================================================================
> --- apr/apr/trunk/network_io/unix/sockaddr.c (original)
> +++ apr/apr/trunk/network_io/unix/sockaddr.c Thu Feb 19 07:15:23 2009
> @@ -564,6 +572,32 @@
> }
> #endif
> }
> + if (family == APR_UNSPEC && hostname && *hostname == '/')
> + family = APR_UNIX;
> + if (family == APR_UNIX) {
> +#if APR_HAVE_SOCKADDR_UN
> + if (hostname && *hostname == '/') {
Why is it needed that hostname always starts with a '/'?
> + *sa = apr_pcalloc(p, sizeof(apr_sockaddr_t));
> + (*sa)->pool = p;
> + apr_cpystrn((*sa)->sa.unx.sun_path, hostname,
> + sizeof((*sa)->sa.unx.sun_path));
> + (*sa)->hostname = apr_pstrdup(p, hostname);
> + (*sa)->family = APR_UNIX;
> + (*sa)->sa.unx.sun_family = APR_UNIX;
> + (*sa)->salen = sizeof(struct sockaddr_un);
> + (*sa)->addr_str_len = sizeof((*sa)->sa.unx.sun_path);
> + (*sa)->ipaddr_ptr = &((*sa)->sa.unx.sun_path);
> + (*sa)->ipaddr_len = (*sa)->addr_str_len;
> +
> + return APR_SUCCESS;
> + }
> + else
> +#endif
> + {
> + *sa = NULL;
> + return APR_ENOTIMPL;
> + }
> + }
> #if !APR_HAVE_IPV6
> /* What may happen is that APR is not IPv6-enabled, but we're still
> * going to call getaddrinfo(), so we have to tell the OS we only
> @@ -616,6 +650,12 @@
> tmphostname, sizeof(tmphostname), NULL, 0,
> flags != 0 ? flags : NI_NAMEREQD);
> }
> +#if APR_HAVE_SOCKADDR_UN
> + else if (sockaddr->family == APR_UNIX) {
> + *hostname = sockaddr->hostname;
Shouldn't we do
apr_pstrdup(sockaddr->pool, sockaddr->hostname)
to protect our internal hostname in sockaddr?
> + return APR_SUCCESS;
> + }
> +#endif
> else
> #endif
> rc = getnameinfo((const struct sockaddr *)&sockaddr->sa, sockaddr->salen,
>
> Modified: apr/apr/trunk/network_io/unix/sockets.c
> URL: http://svn.apache.org/viewvc/apr/apr/trunk/network_io/unix/sockets.c?rev=745763&r1=745762&r2=745763&view=diff
> ==============================================================================
> --- apr/apr/trunk/network_io/unix/sockets.c (original)
> +++ apr/apr/trunk/network_io/unix/sockets.c Thu Feb 19 07:15:23 2009
> @@ -26,12 +26,37 @@
> #define close closesocket
> #endif /* BEOS_R5 */
>
> -static char generic_inaddr_any[16] = {0}; /* big enough for IPv4 or IPv6 */
> +#if APR_HAVE_SOCKADDR_UN
> +#define GENERIC_INADDR_ANY_LEN sizeof(struct sockaddr_un)
Do we know that sizeof(struct sockaddr_un) is always >= 16?
> +#else
> +#define GENERIC_INADDR_ANY_LEN 16
> +#endif
> +
> +/* big enough for IPv4, IPv6 and optionaly sun_path */
> +static char generic_inaddr_any[GENERIC_INADDR_ANY_LEN] = {0};
>
> static apr_status_t socket_cleanup(void *sock)
> {
> apr_socket_t *thesocket = sock;
>
> +#if APR_HAVE_SOCKADDR_UN
> + if (thesocket->bound && thesocket->local_addr->family == APR_UNIX) {
> + /* XXX: Check for return values ? */
> + unlink(thesocket->local_addr->hostname);
Shouldn't we close the socket before unlinking?
> + }
> +#endif
> + if (close(thesocket->socketdes) == 0) {
> + thesocket->socketdes = -1;
> + return APR_SUCCESS;
> + }
> + else {
> + return errno;
> + }
> +}
> +
> +static apr_status_t socket_child_cleanup(void *sock)
> +{
> + apr_socket_t *thesocket = sock;
> if (close(thesocket->socketdes) == 0) {
> thesocket->socketdes = -1;
> return APR_SUCCESS;
Regards
Rüdiger
Re: svn commit: r745763 - in /apr/apr/trunk: ./ build/ include/ include/arch/unix/
network_io/unix/ test/
Posted by Mladen Turk <mt...@apache.org>.
Ruediger Pluem wrote:
>
>> + if (family == APR_UNSPEC && hostname && *hostname == '/')
>> + family = APR_UNIX;
>> + if (family == APR_UNIX) {
>> +#if APR_HAVE_SOCKADDR_UN
>> + if (hostname && *hostname == '/') {
>
> Why is it needed that hostname always starts with a '/'?
>
Right, the second test is not needed.
The first one with APR_UNSPEC is, cause that's the
only (simple enough) way to determine that the address is AF_UNIX one.
>> +#if APR_HAVE_SOCKADDR_UN
>> + else if (sockaddr->family == APR_UNIX) {
>> + *hostname = sockaddr->hostname;
>
> Shouldn't we do
>
> apr_pstrdup(sockaddr->pool, sockaddr->hostname)
>
> to protect our internal hostname in sockaddr?
>
Hmm, probably.
>>
>> -static char generic_inaddr_any[16] = {0}; /* big enough for IPv4 or IPv6 */
>> +#if APR_HAVE_SOCKADDR_UN
>> +#define GENERIC_INADDR_ANY_LEN sizeof(struct sockaddr_un)
>
> Do we know that sizeof(struct sockaddr_un) is always >= 16?
>
Think that's always true. It's always > 100
We can use APR_ALIGN(sizeof(struct sockaddr_un), 16)
if you think that's not the case.
>> +#else
>> +#define GENERIC_INADDR_ANY_LEN 16
>> +#endif
>> +
>> +/* big enough for IPv4, IPv6 and optionaly sun_path */
>> +static char generic_inaddr_any[GENERIC_INADDR_ANY_LEN] = {0};
>>
>> static apr_status_t socket_cleanup(void *sock)
>> {
>> apr_socket_t *thesocket = sock;
>>
>> +#if APR_HAVE_SOCKADDR_UN
>> + if (thesocket->bound && thesocket->local_addr->family == APR_UNIX) {
>> + /* XXX: Check for return values ? */
>> + unlink(thesocket->local_addr->hostname);
>
> Shouldn't we close the socket before unlinking?
>
IMO that's the same.
<snip>
If the name referred to a socket, fifo or device the name for it
is removed but processes which have the object open may continue
to use it.
</snip>
Thus IMO it's always better first to unlink and then close,
preventing race conditions (hard to happen in this tight loop
anyhow, but ...).
Regards
--
^(TM)