Arlie Davis wrote:
> [[[
> Adds support to svnserve to run as a native Windows service. This allows
> svnserve to start automatically, at system boot time, without having a user
> logged in and without the need for an external service 'wrapper' program.
>
> Usage is described in detail in notes/windows-service.txt. Design notes
> are in comments in the source code.
>
> ======================================================================
>
> * subversion/svnserve/main.c
> (main): updated to support running as a Windows service
> Adds new --service run mode
>
> * subversion/svnserve/svn_winservice.h: Added
> Contains public declarations for service support
>
> * subversion/svnserve/svn_winservice.h: Added
> Contains implementation of service support
>
Surely you mean svn_winservice.c? And why the svn_ prefix in the
filenames? This is a private header specific to svnserve; it's not
public, so the svn_ isn't necessary.
> * notes/windows-service.txt: Added
> Contains notes on how to use service support (install, start, stop,
> uninstall)
>
Hm, I think parts of this should ideally go into INSTALL. Or at least be
linked from there.
> Index: notes/windows-service.txt
> ===================================================================
> --- notes/windows-service.txt (revision 0)
> +++ notes/windows-service.txt (revision 0)
> @@ -0,0 +1,203 @@
> +
> +Windows Service Support for svnserve
> +------------------------------------
> +
> +svnserve can now be run as a native Windows service. This means that the
> +service can be started at system boot, or at any other time, without the
> +need for any wrapper code to start the service. The service can be managed
> +like any other Windows service, using command-line tools ("net start",
> +"net stop", or sc.exe) or GUI tools (the Services administrative tool).
> +
> +
> +Installation
> +------------
> +
> +For now, no means is provided to install the service. Windows XP and
> +Windows 2003 Server provide a command-line tool for installing services.
> +To create a service for svnserve, invoke SC.EXE like so:
>
So do WInows NT 4 and Win2k, fwiw.
> + sc create <name>
> + binpath= "c:\svn\bin\svnserve.exe --service <svn-args>"
> + depend= Tcpip
>
What about qoting in the name of the binary itself? For example, if my
svnserve is installed in
C:\Program Files\Subversion\bin\svnserve.exe
(which happens to be the default location proposed by the installer),
how does sc tell where the program name ends and the parameters begin?
[snip]
> +You can create as many services as you like; there is no restriction on
> +the number of services, or their names. I use a prefix, like "svn.foo",
> +"svn.bar", etc. Each service runs in a separate process.
>
But each process should use a different --listen-port and/or
--listen-host. I'd mention that; With Windows "sysadmins", you never
know if they'll grok that themselves or not. :)
[snip]
> +Known Issues
> +------------
> +
> +* No management tool (installer, etc.). For the first revision,
It's a version, not a revision (of an older version :)
[snip]
> +Contact
> +-------
> +
> +This support was written by Arlie Davis, February 2006. You can reach me
> +at my permanent email address, arlie@sublinear.org, and also at my current
> +work email address at, adavis@stonestreetone.com.
>
Um. No. The contact is users@subversion.tigris.org. We don't have
feature maintainers on this project.
A general note for all files, source or otherwise: keep the length of
the lines below 80 columns. I won't repeat that below, but there are
tons of places where you went over that limit.
I get the feeling that you didn't read hacking.html very carefully.
> Index: subversion/svnserve/main.c
> ===================================================================
> --- subversion/svnserve/main.c (revision 18609)
> +++ subversion/svnserve/main.c (working copy)
> @@ -2,7 +2,7 @@
> * main.c : Main control function for svnserve
> *
> * ====================================================================
> - * Copyright (c) 2000-2004 CollabNet. All rights reserved.
> + * Copyright (c) 2000-2006 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
> @@ -41,6 +41,7 @@
> #include "svn_version.h"
>
> #include "svn_private_config.h"
> +#include "svn_winservice.h"
>
> #ifdef HAVE_UNISTD_H
> #include <unistd.h> /* For getpid() */
> @@ -63,6 +64,9 @@
> run_mode_daemon,
> run_mode_tunnel,
> run_mode_listen_once
> +#ifdef ENABLE_WINDOWS_SERVICE_SUPPORT
> + ,run_mode_service
> +#endif
> };
>
This define *should* have a SVN_ prefix, but there's no reason to make
the presence of the enum constant conditional. Notice that we don't
conditionally define connection_mode_fork, even if it's only used if
APR_HAS_FORK.
Besides, you don't actually need a special define; #ifdef WIN32 will do
just nicely, here and everywhere else. Just make the service-specific
functions check for WIn9x and return an error if the service stuff isn't
supported.
> #if APR_HAS_FORK
> @@ -86,6 +90,56 @@
>
> #endif
>
> +#ifdef ENABLE_WINDOWS_SERVICE_SUPPORT
> +/*
> + Ok, I KNOW this is horrible, but I don't see any other way (currently)
> + to safely signal the main thread to stop.
> +
> +*/
> +#include <arch/win32/apr_arch_networkio.h>
> +
> +static SOCKET winservice_accept_socket_handle;
>
Nope. See apr_os_socket_t and apr_os_sock_get in apr_portable.h.
[snip]
> +This isn't pretty, but it's better than a lot of other options. Currently,
> +there is no "right" way to shut down svnserve.
> +
> +*/
> +void winservice_notify_stop()
>
Start the function name on a separate line.
Why is this function in main.c? It should go into the service-specific file.
Why do you dynamically load ws2_32.dll? We already link with winsock,
because APR needs it, so all the socket functions should be available.
[snip]
> /* Option codes and descriptions for svnserve.
> *
> * The entire list must be terminated with an entry of nulls.
> @@ -99,6 +153,9 @@
> #define SVNSERVE_OPT_TUNNEL_USER 259
> #define SVNSERVE_OPT_VERSION 260
> #define SVNSERVE_OPT_PID_FILE 261
> +#ifdef ENABLE_WINDOWS_SERVICE_SUPPORT
> +#define SVNSERVE_OPT_SERVICE 262
> +#endif
>
Define such constants unconditionally.
[snip]
> +#ifdef ENABLE_WINDOWS_SERVICE_SUPPORT
> + /* At this point, the service is "running". Notify the SCM. */
> + winservice_accept_socket_handle = sock->socketdes;
>
This is where you should use apr_os_sock_get.
> + if (run_mode == run_mode_service)
> + winservice_running();
> +#endif
> +
> while (1)
> {
> +#ifdef ENABLE_WINDOWS_SERVICE_SUPPORT
> + if (winservice_is_stopping())
> + {
> + dbg_print("main: exiting");
> + exit(0);
> + }
> +#endif
>
Actually, I'd put all this conditional stuff into the service-specific
implementation. Make the non-windows versions of the functions (e.g.,
winservice_is_stopping()) return something that will work correctly on
other platforms. This reduces clutter int he code and makes it easier to
read.
[snip]
> Index: subversion/svnserve/svn_winservice.c
> ===================================================================
> --- subversion/svnserve/svn_winservice.c (revision 0)
> +++ subversion/svnserve/svn_winservice.c (revision 0)
> @@ -0,0 +1,539 @@
> +/*
> + * svn_winservice.c : Implementation of Windows Service support
>
Just call it winservice.c, without the svn_ prefix.
[snip]
> + *
> + * ====================================================================
> + * Copyright (c) 2000-2006 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/.
> + * ====================================================================
> + */
> +
> +
> +
> +#define APR_WANT_STRFUNC
> +#include <apr_want.h>
> +#include <apr_general.h>
> +#include <apr_getopt.h>
> +#include <apr_network_io.h>
> +#include <apr_signal.h>
> +#include <apr_thread_proc.h>
> +#include <apr_errno.h>
> +
> +#include <locale.h>
> +
> +#include "svn_cmdline.h"
> +#include "svn_types.h"
> +#include "svn_pools.h"
> +#include "svn_error.h"
> +#include "svn_ra_svn.h"
> +#include "svn_utf.h"
> +#include "svn_path.h"
> +#include "svn_opt.h"
> +#include "svn_repos.h"
> +#include "svn_fs.h"
> +#include "svn_version.h"
> +
> +#include "svn_private_config.h"
> +#include "svn_winservice.h"
>
Ho hum. Do you actually *need* all of these defines? It seems to me that
you just duplicated them from another file, without thinking about it
too much. You certainly don't need locale.h, and probably not
svn_cmdline, svn_opt, svn_repos or svn_fs, either. Please clean this up.
> +Design Notes
> +------------
>
Please follow our documentation rules. Most of what you wrote below
should be in docstrings in front of the relevant declaration, not in a
big glob of text at the beginning of the file.
> +
> +The code in this file allows svnserve to run as a Windows service.
> +Windows Services are only supported on operating systems derived
> +from Windows NT, which is basically all modern versions of Windows
> +(2000, XP, Server, Vista, etc.) and excludes the Windows 9x line.
> +
> +Windows Services are processes that are started and controlled by
> +the Service Control Manager. When the SCM wants to start a service,
> +it creates the process, then waits for the process to connect to
> +the SCM, so that the SCM and service process can communicate.
> +This is done using the StartServiceCtrlDispatcher function.
> +
> +In order to minimize changes to the svnserve startup logic, this
> +implementation differs slightly from most service implementations.
> +In most services, main() immediately calls StartServiceCtrlDispatcher,
> +which does not return control to main() until the SCM sends the
> +"stop" request to the service, and the service stops.
>
_This_ part is O.K.
> +
> +
> +Installing the Service
> +---------------------
> +
> +Installation is beyond the scope of source code comments. There
> +is a separate document that describes how to install and uninstall
> +the service. Basically, you create a Windows Service, give it a
> +binary path that points to svnserve.exe, and make sure that you
> +specify --service on the command line.
>
Who cares? We have a separate file in notes/, and --service is
documented in main().
Most of the rest is either redundant or in the wrong place.
[snip]
> +Arlie Davis
> +arlie@sublinear.org
>
As I said before, we do not put author names in the source files. We
have "svn log" for that.
> +#ifdef ENABLE_WINDOWS_SERVICE_SUPPORT
> +
> +#include <assert.h>
> +#include <winsvc.h>
> +#include <tchar.h>
> +
> +/*
> +
> +This is just a placeholder, and doesn't actually constrain the service name.
> +You have to provide *some* service name to the SCM API, but for services that
> +are marked SERVICE_WIN32_OWN_PROCESS, the service name is ignored. It *is*
> +relevant for service binaries that run more than one service in a single
> +process.
> +
> +*/
> +#define WINSERVICE_SERVICE_NAME _("svnserve")
>
If this is just a placeholder, why mark it for translation?
> +Global Variables
> +----------------
>
Again, the docstrings should go with the declarations, not in a separate
comment block.
Of course, we tend to shy away from global data in Subversion, unless
it's completely unavoidable. If I remember my service hacking days, some
of it in fact _is_ unavoidable, but please make sure you're not using
any unnecessary global data.
[snip]
> +void dbg_print(const char* format, ...)
>
Uh-oh ... why isn't this declared "static"?
> +{
> + va_list arglist;
> + char buffer[0x100];
>
Please, no on-stack, fixed-size buffers in our code.
> + size_t len;
> +
> + va_start(arglist, format);
> + _vsnprintf(buffer, 0x100, format, arglist);
>
Use apr_snprintf and let it allocate stuff in a pool. Yes, that means
you have to carry a pool around.
> + va_end(arglist);
> +
> + len = strlen(buffer);
>
Buffer overrun. If the output of _vsnprintf would be longer than the
buffer, it doesn't write a terminating '\0'.
> + while (len > 0 && (buffer[len - 1] == '\r' || buffer[len - 1] == '\n'))
> + buffer[--len] = 0;
>
Sorry? Why are you doing this? Don't you have complete control over what
goes in?
> +
> + if (len+1 < 0x100) {
> + buffer[len++] = '\r';
> + buffer[len] = 0;
> + }
> +
> + if (len+1 < 0x100) {
> + buffer[len++] = '\n';
> + buffer[len] = 0;
> + }
> +
> + OutputDebugStringA(buffer);
> +}
>
Anyway, I do *not* like such uncontrolled "logging". At least, this
dbg_print should be defined only in maintainer mode (#ifdef SVN_DEBUG,
IIRC).
I also wish the _contents_ of the debug prints were more consistent.
[snip]
> +svn_error_t* winservice_start()
> +{
> + HANDLE handles[2];
> + DWORD thread_id;
> + DWORD error_code;
> + apr_status_t apr_status;
> + DWORD wait_status;
> +
> + dbg_print("winservice_start: starting svnserve as a service...");
> +
> + ZeroMemory(&winservice_status, sizeof(winservice_status));
> +
> + winservice_status.dwServiceType = SERVICE_WIN32_OWN_PROCESS;
> + winservice_status.dwControlsAccepted = SERVICE_ACCEPT_STOP;
> + winservice_status.dwCurrentState = SERVICE_STOPPED;
> +
> + /* Create the event that will wake up this thread when we receive SERVICE_START control message. */
> + winservice_start_event = CreateEvent(NULL, FALSE, FALSE, NULL);
> + if (winservice_start_event == NULL)
> + {
> + apr_status = apr_get_os_error();
> + return svn_error_wrap_apr(apr_status, _("Failed to create winservice_start_event"));
> + }
> +
> + winservice_dispatcher_thread = (HANDLE)_beginthreadex(NULL, 0, winservice_dispatcher_thread_routine, NULL, 0, &thread_id);
> + if (winservice_dispatcher_thread == NULL)
> + {
> + apr_status = apr_get_os_error();
> + winservice_cleanup();
> + return svn_error_wrap_apr(apr_status, "The service failed to start");
> + }
>
Hm hm hm, it seems to me that you could convert most of the global data
you're using to an allocated struct, and just send a reference to that
struct to teh dispatcher thread.
[snip]
> + CloseHandle(winservice_dispatcher_thread);
> + winservice_dispatcher_thread = NULL;
> +
> + winservice_cleanup();
>
Why do you close the handle, then call winservice_cleanup which will do
that anyway?
THere are several instances of this pattern.
[snip]
> Index: subversion/svnserve/svn_winservice.h
> ===================================================================
> --- subversion/svnserve/svn_winservice.h (revision 0)
> +++ subversion/svnserve/svn_winservice.h (revision 0)
> @@ -0,0 +1,33 @@
> +/*
> + * svn_winservice.h : Public definitions for Windows Service support
> + *
> + * ====================================================================
> + * Copyright (c) 2000-2004 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/.
> + * ====================================================================
> + */
> +
> +
> +
> +#ifdef _WIN32
> +#define ENABLE_WINDOWS_SERVICE_SUPPORT
>
We use WIN32, not _WIN32, and this define is obviously not necessary.
> +#endif
> +
> +
> +
> +extern svn_error_t* winservice_start();
> +extern void winservice_stop(DWORD exit_code);
> +extern void winservice_running();
> +extern void winservice_notify_stop();
>
No, not extern. They're just prototypes.
> +void dbg_print(const char* format, ...);
> +int winservice_is_stopping();
>
These have no business being in the header, if they're functions local
to winservice.c.
I certainly _hope_ you meant dbg_print to be local to the implementation!
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Feb 24 23:49:15 2006