You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Greg Stein <gs...@gmail.com> on 2008/10/19 17:06:53 UTC
Re: svn commit: r33748 - branches/issue-2382/subversion/svnserve
Why a separate branch for this? It seems that this could be
incrementally add in trunk without a major disruption.
Cheers,
-g
On Sat, Oct 18, 2008 at 6:39 PM, <st...@tigris.org> wrote:
> Author: stsp
> Date: Sat Oct 18 18:39:28 2008
> New Revision: 33748
>
> Log:
> On the issue-2382 branch, add first steps towards support for listening
> on multiple sockets (which may use IPv4 or IPv6) in svnserve.
>
> * subversion/svnserve/server.h
> (init_listeners, wait_for_client): New functions.
>
> * subversion/svnserve/listen.c: New file.
>
> * subversion/svnserve/main.c
> (SVNSERVE_OPT_LISTEN): New option --listen.
> (svnserve__options): Deprecate --listen-host and --listen-port.
> Handle new option --listen, which understands the same address
> formats as apr_parse_addr_port().
> (main): Maintain a list of listen addresses instead of just a single
> host/port pair. Allow an arbitrary number of --listen options
> to build up list of address/port pairs to listen on.
> Provide backwards compatibility with the old --listen-host and
> --listen-port options.
> Make use of API provided by listen.c to manage listening sockets.
>
> Added:
> branches/issue-2382/subversion/svnserve/listen.c
> Modified:
> branches/issue-2382/subversion/svnserve/main.c
> branches/issue-2382/subversion/svnserve/server.h
>
> Added: branches/issue-2382/subversion/svnserve/listen.c
> URL: http://svn.collab.net/viewvc/svn/branches/issue-2382/subversion/svnserve/listen.c?pathrev=33748
> ==============================================================================
> --- /dev/null 00:00:00 1970 (empty, because file is newly added)
> +++ branches/issue-2382/subversion/svnserve/listen.c Sat Oct 18 18:39:28 2008 (r33748)
> @@ -0,0 +1,165 @@
> +/*
> + * listen.c : Management of incoming connections.
> + *
> + * ====================================================================
> + * Copyright (c) 2008 CollabNet. All rights reserved.
> + *
> + * This software is licensed as described in the file COPYING, which
> + * you should have received as part of this distribution. The terms
> + * are also available at http://subversion.tigris.org/license-1.html.
> + * If newer versions of this license are posted there, you may use a
> + * newer version instead, at your option.
> + *
> + * This software consists of voluntary contributions made by many
> + * individuals. For exact contribution history, see the revision
> + * history and logs, available at http://subversion.tigris.org/.
> + * ====================================================================
> + */
> +
> +#include <apr_tables.h>
> +#include <apr_network_io.h>
> +
> +#include "svn_types.h"
> +#include "svn_pools.h"
> +#include "svn_error.h"
> +#include "svn_private_config.h"
> +
> +#include "server.h"
> +
> +struct listener {
> + apr_socket_t *sock;
> + apr_sockaddr_t *sa;
> +};
> +
> +/* Number of outstanding connections allowed in the sockets listen queue.
> + * This queue is managed by APR and does not really concern us much. */
> +#define CONNECTION_BACKLOG 7
> +
> +static apr_array_header_t *listeners;
> +
> +svn_error_t* init_listeners(apr_array_header_t *addresses,
> + apr_pool_t *pool)
> +{
> + apr_status_t status;
> + int i;
> + apr_pool_t *iterpool;
> + int family = AF_INET;
> + apr_socket_t *sock;
> +
> + /* If no addresses were specified, error out. */
> + SVN_ERR_ASSERT(addresses->nelts > 0);
> +
> + iterpool = svn_pool_create(pool);
> + listeners = apr_array_make(pool, 1, sizeof(struct listener));
> +
> +#if APR_HAVE_IPV6
> + /* Make sure we have IPV6 support first before giving apr_sockaddr_info_get
> + APR_UNSPEC, because it may give us back an IPV6 address even if we can't
> + create IPV6 sockets. */
> +#ifdef MAX_SECS_TO_LINGER
> + /* ### old APR interface */
> + status = apr_socket_create(&sock, APR_INET6, SOCK_STREAM, pool);
> +#else
> + status = apr_socket_create(&sock, APR_INET6, SOCK_STREAM, APR_PROTO_TCP,
> + pool);
> +#endif
> + if (status == APR_SUCCESS)
> + {
> + apr_socket_close(sock);
> + family = APR_UNSPEC;
> + }
> +#endif
> +
> + for (i = 0; i < addresses->nelts; i++)
> + {
> + const char *host;
> + char *addr, *scope_id;
> + apr_port_t port;
> + apr_sockaddr_t *sa;
> + struct listener *listener;
> +
> + svn_pool_clear(iterpool);
> +
> + host = APR_ARRAY_IDX(addresses, i, const char*);
> + status = apr_parse_addr_port(&addr, &scope_id, &port, host, iterpool);
> + if (status)
> + return svn_error_wrap_apr(status, _("Cannot parse address '%s'"), host);
> +
> + if (addr == NULL)
> + return svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
> + _("Cannot parse address '%s'"), host);
> + if (port == 0)
> + port = SVN_RA_SVN_PORT;
> +
> + status = apr_sockaddr_info_get(&sa, addr, family, port, 0, pool);
> + if (status)
> + return svn_error_wrap_apr(status, _("Can't get address info"));
> +
> + /* Process all addresses returned by apr_sockaddr_info_get()
> + * and create a listener for each. */
> + while (sa)
> + {
> + int sock_family;
> +
> +#if APR_HAVE_IPV6
> + /* Make sure we're trying to bind to an address with the
> + * correct family. */
> + if (sa->family == AF_INET6)
> + sock_family = APR_INET6;
> + else
> +#endif
> + sock_family = APR_INET;
> +
> +#ifdef MAX_SECS_TO_LINGER
> + /* ### old APR interface */
> + status = apr_socket_create(&sock, sock_family, SOCK_STREAM, pool);
> +#else
> + status = apr_socket_create(&sock, sock_family, SOCK_STREAM,
> + APR_PROTO_TCP, pool);
> +#endif
> + if (status)
> + return svn_error_wrap_apr(status, _("Can't create server socket"));
> +
> + /* Prevents "socket in use" errors when server is killed and quickly
> + * restarted. */
> + apr_socket_opt_set(sock, APR_SO_REUSEADDR, 1);
> +
> + status = apr_socket_bind(sock, sa);
> + if (status)
> + return svn_error_wrap_apr(status, _("Can't bind server socket"));
> +
> + /* Set up the listener. */
> + listener = apr_palloc(pool, sizeof(struct listener));
> + listener->sock = sock;
> + listener->sa = sa;
> + APR_ARRAY_PUSH(listeners, struct listener *) = listener;
> +
> + sa = sa->next;
> + }
> + }
> +
> + return SVN_NO_ERROR;
> +}
> +
> +svn_error_t *
> +wait_for_client(apr_socket_t **sock, apr_pool_t *pool)
> +{
> + struct listener *listener;
> +
> + /* If we have no listener yet, error out. */
> + SVN_ERR_ASSERT(listeners->nelts > 0);
> +
> + /* If we only have one listener, we can let apr_socket_listen() do our job. */
> + if (listeners->nelts == 1)
> + {
> + listener = APR_ARRAY_IDX(listeners, 0, struct listener *);
> + apr_socket_listen(listener->sock, CONNECTION_BACKLOG);
> + *sock = listener->sock;
> + return SVN_NO_ERROR;
> + }
> +
> + /* TODO: multiple addresses */
> + SVN_ERR_ASSERT(FALSE);
> +
> + return SVN_NO_ERROR;
> +}
>
> Modified: branches/issue-2382/subversion/svnserve/main.c
> URL: http://svn.collab.net/viewvc/svn/branches/issue-2382/subversion/svnserve/main.c?pathrev=33748&r1=33747&r2=33748
> ==============================================================================
> --- branches/issue-2382/subversion/svnserve/main.c Sat Oct 18 18:36:49 2008 (r33747)
> +++ branches/issue-2382/subversion/svnserve/main.c Sat Oct 18 18:39:28 2008 (r33748)
> @@ -136,7 +136,8 @@ void winservice_notify_stop(void)
> #define SVNSERVE_OPT_PID_FILE 261
> #define SVNSERVE_OPT_SERVICE 262
> #define SVNSERVE_OPT_CONFIG_FILE 263
> -#define SVNSERVE_OPT_LOG_FILE 264
> +#define SVNSERVE_OPT_LOG_FILE 264
> +#define SVNSERVE_OPT_LISTEN 265
>
> static const apr_getopt_option_t svnserve__options[] =
> {
> @@ -155,11 +156,11 @@ static const apr_getopt_option_t svnserv
> N_("read configuration from file ARG")},
> {"listen-port", SVNSERVE_OPT_LISTEN_PORT, 1,
> #ifdef WIN32
> - N_("listen port\n"
> + N_("listen port (deprecated, use --listen)\n"
> " "
> "[mode: daemon, service, listen-once]")},
> #else
> - N_("listen port\n"
> + N_("listen port (deprecated, use --listen)\n"
> " "
> "[mode: daemon, listen-once]")},
> #endif
> @@ -167,12 +168,31 @@ static const apr_getopt_option_t svnserv
> #ifdef WIN32
> N_("listen hostname or IP address\n"
> " "
> + "(deprecated, use --listen)\n"
> + " "
> "[mode: daemon, service, listen-once]")},
> #else
> N_("listen hostname or IP address\n"
> " "
> + "(deprecated, use --listen\n"
> + " "
> + "[mode: daemon, listen-once]")},
> +#endif
> + {"listen", SVNSERVE_OPT_LISTEN, 1,
> +#ifdef WIN32
> + N_("listen on hostname|IP[:port]\n"
> + " "
> + "Can be specified multiple times.\n"
> + " "
> + "[mode: daemon, service, listen-once]")},
> +#else
> + N_("listen on hostname|IP[:port]\n"
> + " "
> + "Can be specified multiple times.\n"
> + " "
> "[mode: daemon, listen-once]")},
> #endif
> +
> #ifdef CONNECTION_HAVE_THREAD_OPTION
> /* ### Making the assumption here that WIN32 never has fork and so
> * ### this option never exists when --service exists. */
> @@ -353,7 +373,8 @@ int main(int argc, const char *argv[])
> svn_boolean_t foreground = FALSE;
> apr_socket_t *sock, *usock;
> apr_file_t *in_file, *out_file;
> - apr_sockaddr_t *sa;
> + apr_array_header_t *addresses;
> + svn_stringbuf_t *buf;
> apr_pool_t *pool;
> apr_pool_t *connection_pool;
> svn_error_t *err;
> @@ -371,9 +392,8 @@ int main(int argc, const char *argv[])
> struct serve_thread_t *thread_data;
> #endif
> enum connection_handling_mode handling_mode = CONNECTION_DEFAULT;
> - apr_uint16_t port = SVN_RA_SVN_PORT;
> + const char *port = NULL;
> const char *host = NULL;
> - int family = APR_INET;
> int mode_opt_count = 0;
> const char *config_filename = NULL;
> const char *pid_filename = NULL;
> @@ -414,6 +434,8 @@ int main(int argc, const char *argv[])
> params.authzdb = NULL;
> params.log_file = NULL;
>
> + addresses = apr_array_make(pool, 1, sizeof(const char *));
> +
> while (1)
> {
> status = apr_getopt_long(os, svnserve__options, &opt, &arg);
> @@ -453,7 +475,7 @@ int main(int argc, const char *argv[])
> break;
>
> case SVNSERVE_OPT_LISTEN_PORT:
> - port = atoi(arg);
> + port = arg;
> break;
>
> case SVNSERVE_OPT_LISTEN_HOST:
> @@ -539,6 +561,10 @@ int main(int argc, const char *argv[])
> pool));
> break;
>
> + case SVNSERVE_OPT_LISTEN:
> + APR_ARRAY_PUSH(addresses, const char*) = arg;
> + break;
> +
> }
> }
> if (os->ind != argc)
> @@ -655,58 +681,59 @@ int main(int argc, const char *argv[])
> }
> #endif /* WIN32 */
>
> - /* Make sure we have IPV6 support first before giving apr_sockaddr_info_get
> - APR_UNSPEC, because it may give us back an IPV6 address even if we can't
> - create IPV6 sockets. */
> -
> + /* Process old --listen-host and --listen-port options.
> + * While they are deprecated, we still allow them and
> + * convert them to an equivalent --listen option. */
> + buf = svn_stringbuf_create("", pool);
> + if (host && port)
> + {
> + svn_stringbuf_appendcstr(buf, host);
> + svn_stringbuf_appendcstr(buf, ":");
> + svn_stringbuf_appendcstr(buf, port);
> + APR_ARRAY_PUSH(addresses, const char *) = buf->data;
> + }
> + else if (host)
> + {
> + svn_stringbuf_appendcstr(buf, host);
> + /* init_listeners() will use the default port if unspecified. */
> + APR_ARRAY_PUSH(addresses, const char *) = buf->data;
> + }
> + else if (port)
> + {
> + /* We just got a port. Bind to this port with the unspecified
> + * address in all available address families. */
> + svn_stringbuf_appendcstr(buf, APR_ANYADDR);
> + svn_stringbuf_appendcstr(buf, ":");
> + svn_stringbuf_appendcstr(buf, port);
> + APR_ARRAY_PUSH(addresses, const char *) = buf->data;
> #if APR_HAVE_IPV6
> -#ifdef MAX_SECS_TO_LINGER
> - /* ### old APR interface */
> - status = apr_socket_create(&sock, APR_INET6, SOCK_STREAM, pool);
> -#else
> - status = apr_socket_create(&sock, APR_INET6, SOCK_STREAM, APR_PROTO_TCP,
> - pool);
> + /* In case you're not familiar with IPv6: The first two colons
> + * represent the unspecified address, while the third colon
> + * separates the address from the port, like in IPv4. */
> + svn_stringbuf_appendcstr(buf, ":::");
> + svn_stringbuf_appendcstr(buf, port);
> + APR_ARRAY_PUSH(addresses, const char *) = buf->data;
> #endif
> - if (status == 0)
> - {
> - apr_socket_close(sock);
> - family = APR_UNSPEC;
> }
> -#endif
>
> - status = apr_sockaddr_info_get(&sa, host, family, port, 0, pool);
> - if (status)
> + if (addresses->nelts == 0)
> {
> - err = svn_error_wrap_apr(status, _("Can't get address info"));
> - return svn_cmdline_handle_exit_error(err, pool, "svnserve: ");
> - }
> -
> -
> -#ifdef MAX_SECS_TO_LINGER
> - /* ### old APR interface */
> - status = apr_socket_create(&sock, sa->family, SOCK_STREAM, pool);
> -#else
> - status = apr_socket_create(&sock, sa->family, SOCK_STREAM, APR_PROTO_TCP,
> - pool);
> + /* No addresses to listen on were provided, so default to
> + * listen on the default port on the unspecified address
> + * in all available address families. */
> + APR_ARRAY_PUSH(addresses, const char *) = APR_ANYADDR;
> +#if APR_HAVE_IPV6
> + APR_ARRAY_PUSH(addresses, const char *) = "::";
> #endif
> - if (status)
> - {
> - err = svn_error_wrap_apr(status, _("Can't create server socket"));
> - return svn_cmdline_handle_exit_error(err, pool, "svnserve: ");
> }
>
> - /* Prevents "socket in use" errors when server is killed and quickly
> - * restarted. */
> - apr_socket_opt_set(sock, APR_SO_REUSEADDR, 1);
> -
> - status = apr_socket_bind(sock, sa);
> - if (status)
> - {
> - err = svn_error_wrap_apr(status, _("Can't bind server socket"));
> - return svn_cmdline_handle_exit_error(err, pool, "svnserve: ");
> - }
> -
> - apr_socket_listen(sock, 7);
> + /* Start accepting connections. */
> + err = init_listeners(addresses, pool);
> + if (err)
> + return svn_cmdline_handle_exit_error(err, pool, "svnserve: ");
> + err = wait_for_client(&sock, pool);
> + if (err)
> + return svn_cmdline_handle_exit_error(err, pool, "svnserve: ");
>
> #if APR_HAS_FORK
> if (run_mode != run_mode_listen_once && !foreground)
>
> Modified: branches/issue-2382/subversion/svnserve/server.h
> URL: http://svn.collab.net/viewvc/svn/branches/issue-2382/subversion/svnserve/server.h?pathrev=33748&r1=33747&r2=33748
> ==============================================================================
> --- branches/issue-2382/subversion/svnserve/server.h Sat Oct 18 18:36:49 2008 (r33747)
> +++ branches/issue-2382/subversion/svnserve/server.h Sat Oct 18 18:39:28 2008 (r33748)
> @@ -145,6 +145,33 @@ void
> log_error(svn_error_t *err, apr_file_t *log_file, const char *remote_host,
> const char *user, const char *repos, apr_pool_t *pool);
>
> +/* Initialise listeners for every address in the ADDRESSES array.
> + * Each address must be a const char* pointing to a string which must
> + * be parsable by apr_parse_addr_port(). See its documentation for a
> + * description of valid input.
> + *
> + * It is an error to call this function without specifying any address.
> + * If a string does not specify a port to listen on, the default port
> + * (SVN_RA_SVN_PORT) will be used.
> + *
> + * Do all allocations in POOL.
> + */
> +svn_error_t* init_listeners(apr_array_header_t *addresses,
> + apr_pool_t *pool);
> +
> +/* Wait for an incoming connection request, and return the socket
> + * the request was made on in *SOCK. apr_socket_accept() should
> + * be called on the returned socket to enable communication with
> + * the client.
> + *
> + * Do all allocations in POOL.
> + *
> + * It is an error to call this function when listeners are not
> + * set up (i.e. you have to call init_listeners() first, see above).
> + */
> +svn_error_t*
> +wait_for_client(apr_socket_t **sock, apr_pool_t *pool);
> +
> #ifdef __cplusplus
> }
> #endif /* __cplusplus */
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: svn-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: svn-help@subversion.tigris.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r33748 - branches/issue-2382/subversion/svnserve
Posted by Greg Stein <gs...@gmail.com>.
On Sun, Oct 19, 2008 at 4:39 PM, Mark Phippard <ma...@gmail.com> wrote:
> On Sun, Oct 19, 2008 at 4:02 PM, Greg Stein <gs...@gmail.com> wrote:
>> On Sun, Oct 19, 2008 at 12:13 PM, Stefan Sperling <st...@elego.de> wrote:
>>> On Sun, Oct 19, 2008 at 10:06:53AM -0700, Greg Stein wrote:
>>>> Why a separate branch for this? It seems that this could be
>>>> incrementally add in trunk without a major disruption.
>>>
>>> Short answer: What's wrong with branchy development?
>>
>> It keeps the software from being used/tested by more people and the
>> buildbots. All the work that I've been doing on the WC is already
>> getting tested, and we recently found a bug when I made a global
>> read-only. If the development was on a branch, then it would not have
>> been noticed so early. And if it had waited until the remerge, then
>> tracking down the problem could have been mired in a morass of other
>> work.
>>
>> Basically, you get zero testing during your development if you do it
>> on a branch.
>
> One thing we talked about at the summit last week (before you arrived)
> was that with the branch for 1.6 coming in the next couple weeks
> people ought to be conservative about what they are doing on trunk and
> try to limit it to stuff they can complete before the branch.
Fair enough.
On IRC, Stefan said he expected the branch to be short-lived, so the
question is somewhat moot.
But. That said, part of my question was also directed at overall
approach. Meta-level, if you will. IMO, we got screwed on 1.5 because
merge-tracking was developed on a branch. Keeping stuff out of the
main-line is simply asking for trouble, so I think if it can be done
on trunk (maybe with a little bit more precaution/work/overhead), then
it SHOULD be done on trunk.
Given that the branch is going to be short-lived, then (again, IMO) it
really should have been done on trunk. But if it will be rolled into
trunk soon, then ... whatever. Let it wrap on the branch, and get
merged back. It does seem like it is at-most maybe a week more of
work.
As I'm sure you've noticed, I'm not doing my work on a branch. The
changes so far have been improvements to svn's core. Prep work.
Isolating those over on a branch doesn't do anybody any good. Yes, it
can easily be argued that it introduces instability, but ANY change
will, no matter where it is done. So that actually argues *more* for
trunk development where more people will see/test the change.
Cheers,
-g
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r33748 - branches/issue-2382/subversion/svnserve
Posted by Mark Phippard <ma...@gmail.com>.
On Sun, Oct 19, 2008 at 4:02 PM, Greg Stein <gs...@gmail.com> wrote:
> On Sun, Oct 19, 2008 at 12:13 PM, Stefan Sperling <st...@elego.de> wrote:
>> On Sun, Oct 19, 2008 at 10:06:53AM -0700, Greg Stein wrote:
>>> Why a separate branch for this? It seems that this could be
>>> incrementally add in trunk without a major disruption.
>>
>> Short answer: What's wrong with branchy development?
>
> It keeps the software from being used/tested by more people and the
> buildbots. All the work that I've been doing on the WC is already
> getting tested, and we recently found a bug when I made a global
> read-only. If the development was on a branch, then it would not have
> been noticed so early. And if it had waited until the remerge, then
> tracking down the problem could have been mired in a morass of other
> work.
>
> Basically, you get zero testing during your development if you do it
> on a branch.
One thing we talked about at the summit last week (before you arrived)
was that with the branch for 1.6 coming in the next couple weeks
people ought to be conservative about what they are doing on trunk and
try to limit it to stuff they can complete before the branch.
--
Thanks
Mark Phippard
http://markphip.blogspot.com/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r33748 - branches/issue-2382/subversion/svnserve
Posted by Greg Stein <gs...@gmail.com>.
On Sun, Oct 19, 2008 at 12:13 PM, Stefan Sperling <st...@elego.de> wrote:
> On Sun, Oct 19, 2008 at 10:06:53AM -0700, Greg Stein wrote:
>> Why a separate branch for this? It seems that this could be
>> incrementally add in trunk without a major disruption.
>
> Short answer: What's wrong with branchy development?
It keeps the software from being used/tested by more people and the
buildbots. All the work that I've been doing on the WC is already
getting tested, and we recently found a bug when I made a global
read-only. If the development was on a branch, then it would not have
been noticed so early. And if it had waited until the remerge, then
tracking down the problem could have been mired in a morass of other
work.
Basically, you get zero testing during your development if you do it
on a branch.
> Longer answer:
>
> Because it currently breaks svnserve, at least on my machine.
> When no --listen is supplied, it will try to bind to multiple
> sockets by default, because I run dual IPv4/IPv6.
> Support for this isn't implemented yet -- instead, it runs
> into assert(false);
But if it was built on trunk, then you might have taken a slightly
different tack: only do multiple listeners if the right switches are
used. Then once you had the bug fixed, then you could have made it on
by default. Development is a bit harder to plan out when on mainline,
but it *does* get tested.
Cheers,
-g
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r33748 - branches/issue-2382/subversion/svnserve
Posted by Stefan Sperling <st...@elego.de>.
On Sun, Oct 19, 2008 at 10:06:53AM -0700, Greg Stein wrote:
> Why a separate branch for this? It seems that this could be
> incrementally add in trunk without a major disruption.
Short answer: What's wrong with branchy development?
Longer answer:
Because it currently breaks svnserve, at least on my machine.
When no --listen is supplied, it will try to bind to multiple
sockets by default, because I run dual IPv4/IPv6.
Support for this isn't implemented yet -- instead, it runs
into assert(false);
So I thought I'd rather put this on a branch until it is ready.
Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org