I was reviewing the issue #1681 patch, and found a couple of very
minor things. Not coding errors, but just wanted to set these down
here so we'd remember, and have a mail to refer to from the issue.
After the review is a bit of discussion.
> Log message:
>
> Make svnserve show an error message and usage if it is run without any
> arguments. Add a new -i/--inet option to make svnserve use
> stdin/stdout (which was the old default). Add a new -F/--foreground
> option to prevent svnserve from daemonizing (which is useful for
> debugging).
Typo: "--inet" should be "--inetd". We can fix if/when we apply.
The "-F" option appears not to exist in the code ("--foreground" does,
however).
> * subversion/svnserve/main.c
>
> (run_mode): New enum.
>
> (svnserve__options): Add -i/--inetd and -F/--foreground.
>
> (main): Use run_mode instead of several booleans. Show error message
> and usage if no mode is selected. Handle the new inetd and
> foreground options.
>
> * subversion/svnserve/svnserve.8
>
> Updated to match the changes to subversion/svnserve/main.c.
All good (except for the "-F"). Of course, the book needs updating
too. This is part of a much larger bunch of book changes (most of
which Ben, Mike, and Fitz are doing on their own time anyway), so
there will be a chance for this to get tweaked appropriately. Just
noting it here.
> Index: subversion/svnserve/svnserve.8
> ===================================================================
> --- subversion/svnserve/svnserve.8 (revision 8142)
> +++ subversion/svnserve/svnserve.8 (working copy)
> @@ -9,16 +9,15 @@
> \fBsvnserve\fP [\fIoptions\fP]
> .SH DESCRIPTION
> \fBsvnserve\fP allows access to Subversion repositories using the svn
> -network protocol. By default, \fBsvnserve\fP serves a single
> -connection using the stdin/stdout file descriptors, as is appropriate
> -for a daemon running out of inetd, but this mode of operation can be
> -altered by options. \fBsvnserve\FP accepts the following options:
> +network protocol. It can both run as a standalone server process, or
> +it can run out of inetd. You must choose a mode of operation when you
> +start \fBsvnserve\fP. The following options are recognized:
> .PP
> .TP 5
> \fB\-d\fP, \fB\-\-daemon\fP
> Causes \fBsvnserve\fP to run in daemon mode. \fBsvnserve\fP
> -backgrounds itself and accepts and serves connections on the svn port
> -(3690, by default).
> +backgrounds itself and accepts and serves TCP/IP connections on the
> +svn port (3690, by default).
> .PP
> .TP 5
> \fB\-\-listen-port\fP=\fIport\fP
> @@ -30,6 +29,17 @@
> which may be either a hostname or an IP address.
> .PP
> .TP 5
> +\fB\-\-foreground\fP
> +When used together with \fB\-d\fP, this option causes \fBsvnserve\fP
> +to stay in the foreground. This option is mainly useful for
> +debugging.
Good, doesn't list "-F" with "--foreground".
> +.PP
> +.TP 5
> +\fB\-i\fP, \fB\-\-inetd\fP
> +Causes \fBsvnserve\fP to use the stdin/stdout file descriptors, as is
> +appropriate for a daemon running out of inetd.
> +.PP
> +.TP 5
> \fB\-h\fP, \fB\-\-help\fP
> Displays a usage summary and exits.
> .PP
> @@ -42,7 +52,7 @@
> .TP 5
> \fB\-t\fP, \fB\-\-tunnel\fP
> Causes \fBsvnserve\fP to run in tunnel mode, which is just like the
> -default mode of operation (serve one connection over stdin/stdout)
> +inetd mode of operation (serve one connection over stdin/stdout)
> except that the connection is considered to be pre-authenticated with
> the username of the current uid. This flag is selected by the client
> when running over a tunnel agent.
>
> Index: subversion/svnserve/main.c
> ===================================================================
> --- subversion/svnserve/main.c (revision 8142)
> +++ subversion/svnserve/main.c (working copy)
> @@ -48,6 +48,15 @@
> connection_mode_single /* One connection at a time in this process */
> };
>
> +/* The mode in which to run svnserve */
> +enum run_mode {
> + run_mode_none,
> + run_mode_inetd,
> + run_mode_daemon,
> + run_mode_tunnel,
> + run_mode_listen_once,
> +};
> +
> #if APR_HAS_FORK
> #if APR_HAS_THREADS
>
> @@ -81,6 +90,7 @@
> */
> #define SVNSERVE_OPT_LISTEN_PORT 256
> #define SVNSERVE_OPT_LISTEN_HOST 257
> +#define SVNSERVE_OPT_FOREGROUND 258
>
> static const apr_getopt_option_t svnserve__options[] =
> {
> @@ -89,7 +99,10 @@
> "listen port (for daemon mode)"},
> {"listen-host", SVNSERVE_OPT_LISTEN_HOST, 1,
> "listen hostname or IP address (for daemon mode)"},
> + {"foreground", SVNSERVE_OPT_FOREGROUND, 0,
Note the SVNSERVE_OPT_FOREGROUND instead of 'F'.
> + "run in foreground (useful for debugging)"},
> {"help", 'h', 0, "display this help"},
> + {"inetd", 'i', 0, "inetd mode"},
> {"root", 'r', 1, "root of directory to serve"},
> {"read-only", 'R', 0, "deprecated; use repository config file"},
> {"tunnel", 't', 0, "tunnel mode"},
> @@ -172,8 +185,8 @@
>
> int main(int argc, const char *const *argv)
> {
> - svn_boolean_t listen_once = FALSE, daemon_mode = FALSE, tunnel_mode = FALSE;
> - svn_boolean_t read_only = FALSE;
> + enum run_mode run_mode = run_mode_none;
> + svn_boolean_t read_only = FALSE, foreground = FALSE;
> apr_socket_t *sock, *usock;
> apr_file_t *in_file, *out_file;
> apr_sockaddr_t *sa;
> @@ -219,9 +232,17 @@
> break;
>
> case 'd':
> - daemon_mode = TRUE;
> + run_mode = run_mode_daemon;
> break;
>
> + case SVNSERVE_OPT_FOREGROUND:
> + foreground = TRUE;
> + break;
> +
> + case 'i':
> + run_mode = run_mode_inetd;
> + break;
> +
> case SVNSERVE_OPT_LISTEN_PORT:
> port = atoi(arg);
> break;
> @@ -231,11 +252,11 @@
> break;
>
> case 't':
> - tunnel_mode = TRUE;
> + run_mode = run_mode_tunnel;
> break;
>
> case 'X':
> - listen_once = TRUE;
> + run_mode = run_mode_listen_once;
> break;
>
> case 'r':
> @@ -263,14 +284,21 @@
> if (os->ind != argc)
> usage(argv[0]);
>
> - if (!daemon_mode && !listen_once)
> + if (run_mode == run_mode_none)
> {
> + fprintf(stdout, "You must specify one of -d, -i, -t and -X!\n");
> + usage(argv[0]);
> + }
Shouldn't that be "or" instead of "and" in the usage message?
> +
> + if (run_mode == run_mode_inetd || run_mode == run_mode_tunnel)
> + {
> apr_pool_cleanup_register(pool, pool, apr_pool_cleanup_null,
> redirect_stdout);
> apr_file_open_stdin(&in_file, pool);
> apr_file_open_stdout(&out_file, pool);
> conn = svn_ra_svn_create_conn(NULL, in_file, out_file, pool);
> - svn_error_clear(serve(conn, root, tunnel_mode, read_only, pool));
> + svn_error_clear(serve(conn, root, run_mode == run_mode_tunnel,
> + read_only, pool));
> exit(0);
> }
>
> @@ -304,7 +332,7 @@
> apr_socket_listen(sock, 7);
>
> #if APR_HAS_FORK
> - if (!listen_once)
> + if (run_mode != run_mode_listen_once && !foreground)
> apr_proc_detach(APR_PROC_DETACH_DAEMONIZE);
>
> apr_signal(SIGCHLD, sigchld_handler);
> @@ -344,12 +372,11 @@
>
> conn = svn_ra_svn_create_conn(usock, NULL, NULL, connection_pool);
>
> - if (listen_once)
> + if (run_mode == run_mode_listen_once)
> {
> err = serve(conn, root, FALSE, read_only, connection_pool);
>
> - if (listen_once && err
> - && err->apr_err != SVN_ERR_RA_SVN_CONNECTION_CLOSED)
> + if (err && err->apr_err != SVN_ERR_RA_SVN_CONNECTION_CLOSED)
> svn_handle_error(err, stdout, FALSE);
> svn_error_clear(err);
Heh, I remember we discussed that redundant conditional there before
-- nice that it got fixed as part of this change.
Now, here is a mail I was *about* to write about this issue. At the
end, you'll see me starting to have second thoughts:
* Issue #1681
Make svnserve require --inetd to use stdin/stdout.
Justification: Make svnserve more user friendly, low risk.
Votes:
+1: bliss, ghudson
+0: cmpilato
-0: gstein, sussman
The change has three parts:
a) Take away the automatic stdin/stdout behavior; running svnserve
with no arguments would now result in a usage message.
b) Add a new option, -i/--inetd, that gives the old stdin/stdout
behavior.
c) Add a new option, --foreground, that causes svnserve to run
in the foreground, for easier debugging.
Of these, (c) is not in any sense an API issue for 1.0, of course. We
just add the option any time after 1.0, no problem.
(a) and (b) taken together are sort of an API, or rather UI, issue.
People will write scripts (or /etc/inetd.conf entries) that don't pass
the -i/--inetd option, then later if we require it, those references
would all break.
However, there's an upgrade strategy: first do (b) right after 1.0.
Then deprecate the no-arguments mode. Announce this in documentation.
At the next major release, get rid of it. Yes, some breakage will
occur, but we can do our best to minimize it (by letting major OS
package maintainers know about the change), and to
[...]
It was at this point that I started thinking to myself:
"Yeah, uh, great, so all these inetd.conf's break, and how do the
sysadmins find out about it? Their Subversion servers stop
working. And how do they diagnose the problem? By checking
syslog? Posting to the users list? Rereading the documentation?
Yuck. None of those are very nice."
Can we think of any upgrade strategy that would avoid such trouble?
If we can, we should take it; otherwise, I guess I'll reluctantly have
to vote +1 :-) (haven't yet, partly because still want to see if we
can come up with another plan, and partly because I haven't tested
the change, though I have reviewed it).
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Jan 8 07:59:26 2004