You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Arlie Davis <ad...@stonestreetone.com> on 2006/02/27 23:16:20 UTC
[PATCH] Next rev of Windows service support (Feb 27)
I've incorporated Branko's feedback, and am resubmitting the Windows service
support patch.
* windows-service.txt updated
* svn_ prefix removed from filenames
* Debug print statements controlled by SVN_DEBUG
* Fewer #ifdefs in main.c
* Clarified install docs on quoting the binpath string (it works)
* All conditional code controlled by _WIN32
* All lines < 80 columns
* Eliminated use of _vsnprintf
* docstrings for exported functions
* Uses apr_os_sock_get instead of directly accessing sockdes field
I welcome additional feedback. However, I would appreciate it if reviewers
would clearly indicate whether a specific comment means that something must
be changed in order for the patch to be accepted, or should be addressed
later, or is just a "comment" and not a "this has got to change".
I searched for info on docstrings and found very little. hacking.html does
not mention docstrings. I wrote comments next to function prototypes in
headers, using the pattern of other header files. If this is not correct,
please advise.
I have intentionally not altered INSTALL, because it is not clear whether I
should add a link to windows-service.txt, or simply move the
windows-service.txt into INSTALL. I think the best thing is to check in
windows-service.txt as is, and then massage the docs later, as people see
fit. This patch is intended to get the basic service support checked in and
available to people, while future patches will work on getting all of the
docs, installation apps, etc. plugged in.
Thanks.
-- arlie
[[[
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/winservice.h: Added
Contains public declarations for service support
* subversion/svnserve/winservice.c: Added
Contains implementation of service support
* notes/windows-service.txt: Added
Contains notes on how to use service support (install, start, stop,
uninstall)
]]]
RE: [PATCH] Next rev of Windows service support (Feb 27)
Posted by Arlie Davis <ad...@stonestreetone.com>.
Thanks, Brane. Glad I could help, and thanks for the review iterations.
-- arlie
-----Original Message-----
From: Branko Čibej [mailto:brane@xbc.nu]
Sent: Sunday, March 12, 2006 4:26 PM
To: Arlie Davis
Cc: dev@subversion.tigris.org
Subject: Re: [PATCH] Next rev of Windows service support (Feb 27)
Arlie Davis wrote:
> I've incorporated Branko's feedback, and am resubmitting the Windows
> service support patch.
>
I made the changes we discussed, and committed this in r18855. Thanks!
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Next rev of Windows service support (Feb 27)
Posted by Branko Čibej <br...@xbc.nu>.
Arlie Davis wrote:
> I've incorporated Branko's feedback, and am resubmitting the Windows service
> support patch.
>
I made the changes we discussed, and committed this in r18855. Thanks!
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Next rev of Windows service support (Feb 27)
Posted by Daniel Rall <dl...@collab.net>.
On Tue, 28 Feb 2006, Branko Čibej wrote:
> Arlie Davis wrote:
...
> >do have various Linux & *BSD machines available. If there is some
> >formatter that does the right thing, I'll run it before submitting
> >my patches in the future.
>
> Emacs (on Windows or elsewhere) will do the right thing in its default
> (GNU) indentation mode. We have an elisp file in our tree that sets up
> the correct parameters.
http://svn.collab.net/repos/svn/trunk/tools/dev/svn-dev.el
;;; Subversion C conventions
(if (eq major-mode 'c-mode)
(progn
(c-add-style "svn" '("gnu" (c-offsets-alist . ((inextern-lang . 0)))))
(c-set-style "svn")))
(setq indent-tabs-mode nil)
(setq angry-mob-with-torches-and-pitchforks t)
Or if you prefer (with the obligatory "ugh" ;) ...
http://svn.collab.net/repos/svn/trunk/tools/dev/svn-dev.vim
--
Daniel Rall
Re: [PATCH] Next rev of Windows service support (Feb 27)
Posted by Branko Čibej <br...@xbc.nu>.
Arlie Davis wrote:
>> Just FYI, the indentation in winservice.c is wrong ... we indent the
>> braces, too, not just the block contents. I can do that automagically
>> before I commit the patch.
>>
>
> Do you have a macro (or whatever) for formatting according to the
> Subversion conventions?
Not as such...
> I do most of my editing on Windows,
So do I. :)
> but I
> do have various Linux & *BSD machines available. If there is some
> formatter that does the right thing, I'll run it before submitting
> my patches in the future.
>
Emacs (on Windows or elsewhere) will do the right thing in its default
(GNU) indentation mode. We have an elisp file in our tree that sets up
the correct parameters.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: [PATCH] Next rev of Windows service support (Feb 27)
Posted by Arlie Davis <ad...@stonestreetone.com>.
> Actually, the repository root path _can_ be the same. You can have
> any number of processes accessing the same repository.
Ahhh, true. I'm used to dealing with SCMs where this is not the case.
> Should be "apr_os_socket_t", not "HANDLE" -- that way, we can reuse
> it on other platforms if/when appropriate.
Ok.
> Question: A bit lower down, you set this variable to NULL if
> apr_os_sock_get fails, but you don't check for NULL here. Is that safe?
Yes, it's safe. Although there's certainly no harm in checking the
value before the call to CloseHandle. HANDLE is exactly analogous
to a file descriptor on Unix, although it has some other information
encoded into it. So, the absolute worst that can happen is that
CloseHandle returns FALSE, with GetLastError() = ERROR_INVALID_HANDLE
(or whatever the code is).
> Just FYI, the indentation in winservice.c is wrong ... we indent the
> braces, too, not just the block contents. I can do that automagically
> before I commit the patch.
Do you have a macro (or whatever) for formatting according to the
Subversion conventions? I do most of my editing on Windows, but I
do have various Linux & *BSD machines available. If there is some
formatter that does the right thing, I'll run it before submitting
my patches in the future.
-- arlie
-----Original Message-----
From: Branko Čibej [mailto:brane@xbc.nu]
Sent: Monday, February 27, 2006 11:58 PM
To: Arlie Davis
Cc: dev@subversion.tigris.org
Subject: Re: [PATCH] Next rev of Windows service support (Feb 27)
Arlie Davis wrote:
> I have intentionally not altered INSTALL, because it is not clear
> whether I should add a link to windows-service.txt, or simply move the
> windows-service.txt into INSTALL. I think the best thing is to check
> in windows-service.txt as is, and then massage the docs later, as
> people see fit. This patch is intended to get the basic service
> support checked in and available to people, while future patches will
> work on getting all of the docs, installation apps, etc. plugged in.
>
Makes sense.
[snip]
> +You can create as many services as you need; 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.
> +As usual, it is your responsbility as an administrator to make sure
> +that no two service instances use the same repository root path, or
> +the same combination of --listen-port and --listen-host.
>
Actually, the repository root path _can_ be the same. You can have any
number of processes accessing the same repository.
[snip]
> +#ifdef _WIN32
> +#include <arch/win32/apr_arch_networkio.h>
>
I don't believe we actually need this include any longer, do we?
> +static HANDLE winservice_accept_socket_handle;
>
Should be "apr_os_socket_t", not "HANDLE" -- that way, we can reuse it on
other platforms if/when appropriate.
[snip]
> +void winservice_notify_stop()
> +{
> + CloseHandle((HANDLE)winservice_accept_socket_handle);
> +}
>
The cast isn't necessary, winservice_accept_socket_handle is already the
correct type.
Question: A bit lower down, you set this variable to NULL if apr_os_sock_get
fails, but you don't check for NULL here. Is that safe?
This looks good. If nobody objects, I'll commit this after some testing.
Just FYI, the indentation in winservice.c is wrong ... we indent the
braces, too, not just the block contents. I can do that automagically
before I commit the patch.
I still think we can do away with most of the new #ifdefs in main.c, and
I'll probably get rid of them in a subsequent commit.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Next rev of Windows service support (Feb 27)
Posted by Branko Čibej <br...@xbc.nu>.
Arlie Davis wrote:
> I have intentionally not altered INSTALL, because it is not clear whether I
> should add a link to windows-service.txt, or simply move the
> windows-service.txt into INSTALL. I think the best thing is to check in
> windows-service.txt as is, and then massage the docs later, as people see
> fit. This patch is intended to get the basic service support checked in and
> available to people, while future patches will work on getting all of the
> docs, installation apps, etc. plugged in.
>
Makes sense.
[snip]
> +You can create as many services as you need; 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. As usual,
> +it is your responsbility as an administrator to make sure that no
> +two service instances use the same repository root path, or the same
> +combination of --listen-port and --listen-host.
>
Actually, the repository root path _can_ be the same. You can have any
number of processes accessing the same repository.
[snip]
> +#ifdef _WIN32
> +#include <arch/win32/apr_arch_networkio.h>
>
I don't believe we actually need this include any longer, do we?
> +static HANDLE winservice_accept_socket_handle;
>
Should be "apr_os_socket_t", not "HANDLE" -- that way, we can reuse it
on other platforms if/when appropriate.
[snip]
> +void winservice_notify_stop()
> +{
> + CloseHandle((HANDLE)winservice_accept_socket_handle);
> +}
>
The cast isn't necessary, winservice_accept_socket_handle is already the
correct type.
Question: A bit lower down, you set this variable to NULL if
apr_os_sock_get fails, but you don't check for NULL here. Is that safe?
This looks good. If nobody objects, I'll commit this after some testing.
Just FYI, the indentation in winservice.c is wrong ... we indent the
braces, too, not just the block contents. I can do that automagically
before I commit the patch.
I still think we can do away with most of the new #ifdefs in main.c, and
I'll probably get rid of them in a subsequent commit.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Next rev of Windows service support (Feb 27)
Posted by Branko Čibej <br...@xbc.nu>.
Malcolm Rowe wrote:
> On Tue, Feb 28, 2006 at 05:48:55PM +0300, Ivan Zhakov wrote:
>
>>> +svn_error_t* winservice_start()
>>>
>> Same as previous: "svn_error_t *winservice_start()"
>>
>>
>
> That, and we usually line-break after the '*':
>
> svn_error_t *
> winservice_start()
>
> I assume this isn't static because the SCM calls it directly?
>
It's not static because it's called from main().
> Shouldn't that be winservice_start(void)? This isn't C++, is it?
>
My goodness, you're right. I missed that.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Next rev of Windows service support (Feb 27)
Posted by Malcolm Rowe <ma...@farside.org.uk>.
On Tue, Feb 28, 2006 at 05:48:55PM +0300, Ivan Zhakov wrote:
> > +svn_error_t* winservice_start()
> Same as previous: "svn_error_t *winservice_start()"
>
That, and we usually line-break after the '*':
svn_error_t *
winservice_start()
I assume this isn't static because the SCM calls it directly?
Shouldn't that be winservice_start(void)? This isn't C++, is it?
> > +
> > +static HANDLE winservice_accept_socket_handle;
> Uninitialized variable.
>
Global variables are guaranteed to be initalised to zero, no?
Regards,
Malcolm
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Next rev of Windows service support (Feb 27)
Posted by Branko Čibej <br...@xbc.nu>.
Ivan Zhakov wrote:
>> +
>> +static HANDLE winservice_accept_socket_handle;
>>
> Uninitialized variable.
>
Actually, it isn't. In C, global data is guaranteed to be
zero-initialized (unless an explicit initializer is provided, of course).
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Next rev of Windows service support (Feb 27)
Posted by Branko Čibej <br...@xbc.nu>.
Arlie Davis wrote:
>> +#include <arch/win32/apr_arch_networkio.h>
>> I consider we don't need this. See below.
>>
>
> It's necessary for the apr_os_get_sock call. I tried removing
> this #include, and it would not compile.
>
apr_os_sock_get is declared in apr_portable.h and defined (out of line)
in network_io/<arch>/sockets.c. You really shouldn't need this
arch-specific include.
> Ivan, Branko - Do you want me to make these edits and resubmit the patch,
> or not? And again, thanks for the review.
>
Not as far as I'm concerned. Since I'll happily do the reformatting of
winservice.c, dong the tweaks we agreed upon here won't take that much
extra time.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Next rev of Windows service support (Feb 27)
Posted by Arlie Davis <ad...@stonestreetone.com>.
> > +static void WINAPI winservice_service_main(DWORD argc, LPTSTR* argv)
> We use "LPTSTR *argv".
Ok.
> > +svn_error_t* winservice_start()
> Same as previous: "svn_error_t *winservice_start()"
Ok. Seems a rather low priority, neh?
[..]
> +void winservice_stop(DWORD exit_code)
> Incorrect comment. main() doesn't call winservice_stop, it been
> called from atexit(). So you should make it static and fix comment.
main() calls it *indirectly*. The comment is meant to indicate
which thread calls the function, not necessarily which function
is the immediate caller. But I agree that a more accurate comment
would help, such as "the main() thread calls ...".
Also, the intent of the design was that main() would call
winservice_stop if main() failed to properly start the service
(open sockets, etc.).
> +#include <arch/win32/apr_arch_networkio.h>
> I consider we don't need this. See below.
It's necessary for the apr_os_get_sock call. I tried removing
this #include, and it would not compile.
> +
> +static HANDLE winservice_accept_socket_handle;
> Uninitialized variable.
Guaranteed to be 0 (NULL) by ANSI C. However, if you want to add
an initializer, fine with me.
> APR doesn't free memory on apr_socket_close(), memory freed when pool
> destroyed. So we can safely close it by apr_socket_close(), with check for
> NULL pointer of course.
The current implementation is guaranteed to avoid all such issues (when
memory may be safely accessed), and is guaranteed to be safe, no
matter what the main() thread is doing. I am opposed to this change.
Ivan, Branko - Do you want me to make these edits and resubmit the patch,
or not? And again, thanks for the review.
-- arlie
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Next rev of Windows service support (Feb 27)
Posted by Ivan Zhakov <ch...@gmail.com>.
On 2/28/06, Arlie Davis <ad...@stonestreetone.com> wrote:
> I've incorporated Branko's feedback, and am resubmitting the Windows service
> support patch.
>
> * windows-service.txt updated
> * svn_ prefix removed from filenames
> * Debug print statements controlled by SVN_DEBUG
> * Fewer #ifdefs in main.c
> * Clarified install docs on quoting the binpath string (it works)
> * All conditional code controlled by _WIN32
> * All lines < 80 columns
> * Eliminated use of _vsnprintf
> * docstrings for exported functions
> * Uses apr_os_sock_get instead of directly accessing sockdes field
>
> I welcome additional feedback. However, I would appreciate it if reviewers
> would clearly indicate whether a specific comment means that something must
> be changed in order for the patch to be accepted, or should be addressed
> later, or is just a "comment" and not a "this has got to change".
>
> I searched for info on docstrings and found very little. hacking.html does
> not mention docstrings. I wrote comments next to function prototypes in
> headers, using the pattern of other header files. If this is not correct,
> please advise.
>
> I have intentionally not altered INSTALL, because it is not clear whether I
> should add a link to windows-service.txt, or simply move the
> windows-service.txt into INSTALL. I think the best thing is to check in
> windows-service.txt as is, and then massage the docs later, as people see
> fit. This patch is intended to get the basic service support checked in and
> available to people, while future patches will work on getting all of the
> docs, installation apps, etc. plugged in.
>
> Thanks.
>
> -- arlie
>
> [[[
>
> 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/winservice.h: Added
> Contains public declarations for service support
>
> * subversion/svnserve/winservice.c: Added
> Contains implementation of service support
>
> * notes/windows-service.txt: Added
> Contains notes on how to use service support (install, start, stop,
> uninstall)
> ]]]
>
Arlie, I have reviewed your patch, see comments inline:
> Index: notes/windows-service.txt
> ===================================================================
> --- notes/windows-service.txt (revision 0)
> +++ notes/windows-service.txt (revision 0)
> @@ -0,0 +1,207 @@
> +
[..skip...]
> +
> Index: subversion/svnserve/winservice.c
> ===================================================================
> --- subversion/svnserve/winservice.c (revision 0)
> +++ subversion/svnserve/winservice.c (revision 0)
> @@ -0,0 +1,485 @@
> +/*
> + * svn_winservice.c : Implementation of Windows Service support
> + *
> + * ====================================================================
> + * 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_errno.h>
> +
> +#include "svn_error.h"
> +
> +#include "svn_private_config.h"
> +#include "winservice.h"
> +
> +/*
> +
> +Design Notes
> +------------
> +
> +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.
> +
> +
> +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.
> +
> +
> +Starting the Service
> +--------------------
> +
> +First, the SCM decides that it wants to start a service. It creates
> +the process for the service, passing it the command-line that is
> +stored in the service configuration (stored in the registry).
> +
> +Next, main() runs. The command-line should contain the --service
> +argument, which is the hint that svnserve is running under the SCM,
> +not as a standalone process. main() calls winservice_start().
> +
> +winservice_start() creates an event object (winservice_start_event),
> +and creates and starts a separate thread, the "dispatcher" thread.
> +winservice_start() then waits for either winservice_start_event
> +to fire (meaning: "the dispatcher thread successfully connected
> +to the SCM, and now the service is starting") or for the dispatcher
> +thread to exit (meaning: "failed to connect to SCM").
> +
> +If the dispatcher thread quit, then winservice_start returns an error.
> +If the start event fired, then winservice_start returns a success code
> +(SVN_NO_ERROR). At this point, the service is now in the "starting"
> +state, from the perspective of the SCM. winservice_start also registers
> +an atexit handler, which handles cleaning up some of the service logic,
> +as explained below in "Stopping the Service".
> +
> +Next, control returns to main(), which performs the usual startup
> +logic for svnserve. Mostly, it creates the listener socket. If
> +main() was able to start the service, then it calls the function
> +winservice_running().
> +
> +winservice_running() informs the SCM that the service has finished
> +starting, and is now in the "running" state. main() then does its
> +work, accepting client sockets and processing SVN requests.
> +
> +Stopping the Service
> +--------------------
> +
> +At some point, the SCM will decide to stop the service, either because
> +an administrator chose to stop the service, or the system is shutting
> +down. To do this, the SCM calls winservice_handler() with the
> +SERVICE_CONTROL_STOP control code. When this happens,
> +winservice_handler() will inform the SCM that the service is now
> +in the "stopping" state, and will call winservice_notify_stop().
> +
> +winservice_notify_stop() is responsible for cleanly shutting down the
> +svnserve logic (waiting for client requests to finish, stopping database
> +access, etc.). Right now, all it does is close the listener socket,
> +which causes the apr_socket_accept() call in main() to fail. main()
> +then calls exit(), which processes all atexit() handlers, which
> +results in winservice_stop() being called.
> +
> +winservice_stop() notifies the SCM that the service is now stopped,
> +and then waits for the dispatcher thread to exit. Because all services
> +in the process have now stopped, the call to StartServiceCtrlDispatcher
> +(in the dispatcher thread) finally returns, and winservice_stop() returns,
> +and the process finally exits.
> +
> +*/
> +
> +
> +#ifdef _WIN32
> +
> +#include <assert.h>
> +#include <winsvc.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 (as is the case for svnserve),
> +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"
> +
> +
> +/* Win32 handle to the dispatcher thread. */
> +static HANDLE winservice_dispatcher_thread = NULL;
> +
> +/* Win32 event handle, used to notify winservice_start() that we have
> + successfully connected to the SCM. */
> +static HANDLE winservice_start_event = NULL;
> +
> +/* RPC handle that allows us to notify the SCM of changes in our
> + service status. */
> +static SERVICE_STATUS_HANDLE winservice_status_handle = NULL;
> +
> +/* Our current idea of the service status (stopped, running,
> + controls accepted, exit code, etc.) */
> +static SERVICE_STATUS winservice_status;
> +
> +
> +#ifdef SVN_DEBUG
> +
> +static void dbg_print(const char* text)
> +{
> + OutputDebugStringA(text);
> +}
> +
> +#else
> +
> +/* Make sure dbg_print compiles to nothing in release builds. */
> +#define dbg_print(text)
> +
> +#endif
> +
> +static void winservice_atexit(void);
> +
> +/* Notifies the Service Control Manager of the current state of the
> + service. */
> +static void winservice_update_state()
> +{
> + if (winservice_status_handle != NULL)
> + {
> + if (!SetServiceStatus(winservice_status_handle, &winservice_status)) {
> + dbg_print("SetServiceStatus - FAILED\r\n");
> + }
> + }
> +}
> +
> +/*
> +
> +This function cleans up state associated with the service support.
> +If the dispatcher thread handle is non-NULL, then this function
> +will wait for the dispatcher thread to exit.
> +
> +*/
> +static void winservice_cleanup()
> +{
> + if (winservice_start_event != NULL)
> + {
> + CloseHandle(winservice_start_event);
> + winservice_start_event = NULL;
> + }
> +
> + if (winservice_dispatcher_thread != NULL)
> + {
> + dbg_print("winservice_cleanup: waiting for dispatcher thread to exit\r\n");
> + WaitForSingleObject(winservice_dispatcher_thread, INFINITE);
> + CloseHandle(winservice_dispatcher_thread);
> + winservice_dispatcher_thread = NULL;
> + }
> +}
> +
> +/*
> +
> +The SCM invokes this function to cause state changes in the service.
> +
> +*/
> +static void WINAPI winservice_handler(DWORD control)
> +{
> + switch (control)
> + {
> + case SERVICE_CONTROL_INTERROGATE:
> + /* The SCM just wants to check our state. We are required to call
> + SetServiceStatus, but we don't need to make any state changes. */
> + dbg_print("SERVICE_CONTROL_INTERROGATE\r\n");
> + winservice_update_state();
> + break;
> +
> + case SERVICE_CONTROL_STOP:
> + dbg_print("SERVICE_CONTROL_STOP\r\n");
> + winservice_status.dwCurrentState = SERVICE_STOP_PENDING;
> + winservice_update_state();
> + winservice_notify_stop();
> + break;
> + }
> +}
> +
> +/*
> +
> +This is the "service main" routine (in the Win32 terminology).
> +
> +Normally, this function (thread) implements the "main" loop of a service.
> +However, in order to minimize changes to the svnserve main() function, this
> +function is running in a different thread, and main() is blocked in
> +winservice_start(), waiting for winservice_start_event. So this function
> +(thread) only needs to signal that event to "start" the service.
> +
> +If this function succeeds, it signals winservice_start_event, which wakes
> +up the winservice_start() frame that is blocked,
> +
> +*/
> +static void WINAPI winservice_service_main(DWORD argc, LPTSTR* argv)
We use "LPTSTR *argv".
> +{
> + DWORD error;
> +
> + assert(winservice_start_event != NULL);
> +
> + winservice_status_handle = RegisterServiceCtrlHandler(
> + WINSERVICE_SERVICE_NAME, winservice_handler);
> + if (winservice_status_handle == NULL)
> + {
> + /* Ok, that's not fair. We received a request to start a service,
> + and now we cannot bind to the SCM in order to update status?
> + Bring down the app. */
> + error = GetLastError();
> + dbg_print("RegisterServiceCtrlHandler FAILED\r\n");
> + /* Put the error code somewhere where winservice_start can find it. */
> + winservice_status.dwWin32ExitCode = error;
> + SetEvent(winservice_start_event);
> + return;
> + }
> +
> + winservice_status.dwCurrentState = SERVICE_START_PENDING;
> + winservice_status.dwWin32ExitCode = ERROR_SUCCESS;
> + winservice_update_state();
> +
> + dbg_print("winservice_service_main: service is starting\r\n");
> + SetEvent(winservice_start_event);
> +}
> +
> +static const SERVICE_TABLE_ENTRY winservice_service_table[] =
> +{
> + { WINSERVICE_SERVICE_NAME, winservice_service_main },
> + { NULL, NULL }
> +};
> +
> +/*
> +
> +This is the thread routine for the "dispatcher" thread. The purpose of
> +this thread is to connect this process with the Service Control Manager,
> +which allows this process to receive control requests from the SCM, and
> +allows this process to update the SCM with status information.
> +
> +The StartServiceCtrlDispatcher connects this process to the SCM. If it
> +succeeds, then it will not return until all of the services running in
> +this process have stopped. (In our case, there is only one service per
> +process.)
> +
> +*/
> +static DWORD WINAPI winservice_dispatcher_thread_routine(PVOID arg)
> +{
> + dbg_print("winservice_dispatcher_thread_routine: starting\r\n");
> +
> + if (!StartServiceCtrlDispatcher(winservice_service_table))
> + {
> + /* This is a common error. Usually, it means the user has
> + invoked the service with the --service flag directly. This is
> + incorrect. The only time the --service flag is passed is when
> + the process is being started by the SCM. */
> + DWORD error = GetLastError();
> +
> + dbg_print("dispatcher: FAILED to connect to SCM\r\n");
> + return error;
> + }
> +
> + dbg_print("dispatcher: SCM is done using this process -- exiting\r\n");
> + return ERROR_SUCCESS;
> +}
> +
> +/* If svnserve needs to run as a Win32 service, then we need to coordinate
> + with the Service Control Manager (SCM) before continuing. This
> + function call registers the svnserve.exe process with the SCM, waits
> + for the "start" command from the SCM (which will come very quickly),
> + and confirms that those steps succeeded.
> +
> + After this call succeeds, the service should perform whatever work it
> + needs to start the service, and then the service should call
> + winservice_running() (if no errors occurred) or winservice_stop()
> + (if something failed during startup). */
> +svn_error_t* winservice_start()
Same as previous: "svn_error_t *winservice_start()"
[..]
> +}
> +
> +/*
> +
> +main() calls this function in order to notify the SCM that the service
> +has stopped. This function also handles cleaning up the dispatcher
> +thread (the one that we created above in winservice_start.
> +
> +*/
> +void winservice_stop(DWORD exit_code)
Incorrect comment. main() doesn't call winservice_stop, it been called
from atexit(). So you should make it static and fix comment.
> +{
> + dbg_print("winservice_stop - notifying SCM that service has stopped\r\n");
> + winservice_status.dwCurrentState = SERVICE_STOPPED;
> + winservice_status.dwWin32ExitCode = exit_code;
> + winservice_update_state();
> +
> + if (winservice_dispatcher_thread != NULL) {
> + dbg_print("waiting for dispatcher thread to exit...\r\n");
> + WaitForSingleObject(winservice_dispatcher_thread, INFINITE);
> + dbg_print("dispatcher thread has exited.\r\n");
> +
> + CloseHandle(winservice_dispatcher_thread);
> + winservice_dispatcher_thread = NULL;
> + } else {
> + /* There was no dispatcher thread. So we never started in
> + the first place. */
> + exit_code = winservice_status.dwWin32ExitCode;
> + dbg_print("dispatcher thread was not running\r\n");
> + }
> +
> + if (winservice_start_event != NULL) {
> + CloseHandle(winservice_start_event);
> + winservice_start_event = NULL;
> + }
> +
> + dbg_print("winservice_stop - service has stopped\r\n");
> +}
> +
> +
> +
> +/*
> +This function is installed as an atexit-handler.
> +This is done so that we don't need to alter every exit() call in main().
> +*/
> +static void winservice_atexit(void)
> +{
> + dbg_print("winservice_atexit - stopping\r\n");
> + winservice_stop(ERROR_SUCCESS);
> +}
> +
> +svn_boolean_t winservice_is_stopping()
> +{
> + return winservice_status.dwCurrentState == SERVICE_STOP_PENDING;
> +}
> +
> +#endif /* _WIN32 */
[skip]
> Index: subversion/svnserve/main.c
> ===================================================================
> --- subversion/svnserve/main.c (revision 18642)
> +++ 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
> @@ -25,6 +25,7 @@
> #include <apr_network_io.h>
> #include <apr_signal.h>
> #include <apr_thread_proc.h>
> +#include <apr_portable.h>
>
> #include <locale.h>
>
> @@ -41,6 +42,7 @@
> #include "svn_version.h"
>
> #include "svn_private_config.h"
> +#include "winservice.h"
>
> #ifdef HAVE_UNISTD_H
> #include <unistd.h> /* For getpid() */
> @@ -62,7 +64,8 @@
> run_mode_inetd,
> run_mode_daemon,
> run_mode_tunnel,
> - run_mode_listen_once
> + run_mode_listen_once,
> + run_mode_service
> };
>
> #if APR_HAS_FORK
> @@ -86,6 +89,41 @@
>
> #endif
>
> +#ifdef _WIN32
> +#include <arch/win32/apr_arch_networkio.h>
I consider we don't need this. See below.
> +
> +static HANDLE winservice_accept_socket_handle;
Uninitialized variable.
> +
> +/*
> +
> +The SCM calls this function (on an arbitrary thread, not the main()
> +thread!) when it wants to stop the service.
> +
> +For now, our strategy is to close the listener socket, in order to
> +unblock main() and cause it to exit its accept loop. We cannot use
> +apr_socket_close, because that function deletes the apr_socket_t structure,
> +as well as closing the socket handle. If we called apr_socket_close here,
> +then main() will also call apr_socket_close, resulting in a double-free.
> +This way, we just close the kernel socket handle, which causes the accept()
> +function call to fail, which causes main() to clean up the socket. So,
> +memory gets freed only once.
APR doesn't free memory on apr_socket_close(), memory freed when pool
destroyed. So we can safely close it by apr_socket_close(), with check for
NULL pointer of course.
> +
> +This isn't pretty, but it's better than a lot of other options. Currently,
> +there is no "right" way to shut down svnserve.
> +
> +We store the OS handle rather than a pointer to the apr_socket_t structure
> +in order to eliminate any possibility of illegal memory access.
> +
> +*/
> +
> +void winservice_notify_stop()
> +{
> + CloseHandle((HANDLE)winservice_accept_socket_handle);
> +}
> +
> +#endif /* _WIN32 */
> +
> +
[skip]
--
Ivan Zhakov