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/24 20:44:14 UTC
[PATCH] Windows service support
[[[
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
* notes/windows-service.txt: Added
Contains notes on how to use service support (install, start, stop,
uninstall)
]]]
RE: [PATCH] Windows service support
Posted by Arlie Davis <ad...@stonestreetone.com>.
Ok. Here's the updated patch, with the svn_ prefix removed, and with line
endings set to \n.
[[[
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
* notes/windows-service.txt: Added
Contains notes on how to use service support (install, start, stop,
uninstall)
]]]
-- arlie
-----Original Message-----
From: Branko Čibej [mailto:brane@xbc.nu]
Sent: Friday, February 24, 2006 4:57 PM
To: Arlie Davis
Cc: 'Greg Hudson'; dev@subversion.tigris.org
Subject: Re: [PATCH] Windows service support
Arlie Davis wrote:
> Fair enough. What do you suggest?
>
Just drop the svn_ prefix on both the header and the implementation.
I'm in the middle of a review of the patch. Coming up soon.
-- Brane
Re: [PATCH] Windows service support
Posted by Branko Čibej <br...@xbc.nu>.
Arlie Davis wrote:
> Fair enough. What do you suggest?
>
Just drop the svn_ prefix on both the header and the implementation.
I'm in the middle of a review of the patch. Coming up soon.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: [PATCH] Windows service support
Posted by Arlie Davis <ad...@stonestreetone.com>.
Fair enough. What do you suggest?
-- arlie
-----Original Message-----
From: Greg Hudson [mailto:ghudson@MIT.EDU]
Sent: Friday, February 24, 2006 4:38 PM
To: Arlie Davis
Cc: dev@subversion.tigris.org
Subject: Re: [PATCH] Windows service support
Hi. Minor tweak: please don't name new source files starting with "svn_".
We do that in a few places (which I find annoying); overall, it's not how
our source file naming scheme works.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Windows service support
Posted by Greg Hudson <gh...@MIT.EDU>.
Hi. Minor tweak: please don't name new source files starting with
"svn_". We do that in a few places (which I find annoying); overall,
it's not how our source file naming scheme works.
---------------------------------------------------------------------
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
Re: [PATCH] Windows service support
Posted by Branko Čibej <br...@xbc.nu>.
Joseph Galbraith wrote:
>> We use WIN32, not _WIN32, and this define is obviously not necessary.
>>
>
> We might want to consider changing that, since it
> appears that WIN32 isn't defined when building for
> 64-bit, but _WIN32 is.
>
IIRC we define WIN32 in our project files.
> In other words, _WIN32 appears to be the correct define
> if you mean #if "this is the win32 api" and WIN32 appears
> to be the correct define if you mean "this is the
> 32-bit version of the win32 api."
>
But yes, the moment we start porting Subversion to Win64 (I haven't
heard of anybody actually building 64-bit Windows binaries yet), we'll
need three symbols instead of one:
* A generic one meaning "this is Windows"
* Two specific ones, one for Win32 and one for Win64.
Although I personally hope we won't ever need the specific versions.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Windows service support
Posted by Stefan Küng <to...@gmail.com>.
Norbert Unterberg wrote:
> 2006/2/25, Joseph Galbraith <ga...@vandyke.com>:
>
>>> We use WIN32, not _WIN32, and this define is obviously not necessary.
>> We might want to consider changing that, since it
>> appears that WIN32 isn't defined when building for
>> 64-bit, but _WIN32 is.
>>
>> In other words, _WIN32 appears to be the correct define
>> if you mean #if "this is the win32 api" and WIN32 appears
>> to be the correct define if you mean "this is the
>> 32-bit version of the win32 api."
>
> It seems to be differernt.
> _WIN32 is the documented predefined macro that is always defined on
> both 32 and 64 bit windows platform.
> _WIN64 is predefined for 64 bit windows only.
> WIN32 is not defined automatically. However, all MS wizard generated
> project files I've seen seem to manually define WIN32 on the command
> line when invoking the compiler. I think this is a relict from early
> windows compilers (used to distinguish between 16 bit and 32 bit code)
> and kept for compatibility with old code.
Attention here: WIN32 isn't defined automatically anymore with VS2005
(and VC-Express). You have to manually add this to compile projects
which rely on that define.
So I'd stay away from WIN32 if possible and use _WIN32 instead.
(that's why we had some big problems compiling apr with VS2005 when we
switched until we patched the build files accordingly).
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.tigris.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Windows service support
Posted by Norbert Unterberg <nu...@gmail.com>.
2006/2/25, Joseph Galbraith <ga...@vandyke.com>:
> > We use WIN32, not _WIN32, and this define is obviously not necessary.
>
> We might want to consider changing that, since it
> appears that WIN32 isn't defined when building for
> 64-bit, but _WIN32 is.
>
> In other words, _WIN32 appears to be the correct define
> if you mean #if "this is the win32 api" and WIN32 appears
> to be the correct define if you mean "this is the
> 32-bit version of the win32 api."
It seems to be differernt.
_WIN32 is the documented predefined macro that is always defined on
both 32 and 64 bit windows platform.
_WIN64 is predefined for 64 bit windows only.
WIN32 is not defined automatically. However, all MS wizard generated
project files I've seen seem to manually define WIN32 on the command
line when invoking the compiler. I think this is a relict from early
windows compilers (used to distinguish between 16 bit and 32 bit code)
and kept for compatibility with old code.
Norbert
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Windows service support
Posted by Joseph Galbraith <ga...@vandyke.com>.
> We use WIN32, not _WIN32, and this define is obviously not necessary.
We might want to consider changing that, since it
appears that WIN32 isn't defined when building for
64-bit, but _WIN32 is.
In other words, _WIN32 appears to be the correct define
if you mean #if "this is the win32 api" and WIN32 appears
to be the correct define if you mean "this is the
32-bit version of the win32 api."
Thanks,
Joseph
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Windows service support
Posted by Branko Čibej <br...@xbc.nu>.
Arlie Davis wrote:
> I don't know if SC.EXE gets this right -- I kind of doubt it. I know for a
> fact that Windows *itself* supports quoted names in the binary path.
> So it's just a matter of creating the service with the right path. I'll
> check
> and see if I can convince SC.EXE to create a service with quotes in the
> binary path.
>
> Earlier, during discussion, it was agreed that the best path was to get
> this basic support committed first, and then deal with installation
> applications. SC.EXE is just a crude "developer" interface to the service
> control APIs, and is not intended to be the "real" interface. I already
> said
> that I will provide a service installer -- in a future patch. For now, we
> can
> document the fact that SC.EXE probably gets confused with double-quotes
> in the binary path.
>
> As such, this criticism (though fair) applies to service installers, not to
> this patch, which deals solely with service support in svnserve itself.
>
I didn't mean it as a criticism, it was merely a question. Mentioning
that there may be problems with such paths is good enough fo me; I see
no need to go overboard writing code to fix things that aren't really
our problem.
>> +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. :)
>>
>
> Please don't pretend or imply that Windows "sysadmins" are not
> "real" system administrators, or are incompetent or are stupid. It's
> offensive and patronizing. I've worked in both environments, and I've
> met plenty of Unix idiots, too.
>
You did see the smiley, didn't you?
>> 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.
>>
>
> That won't work. The service-specific APIs are not even present on Win 9x.
> I would rather not have to use run-time DLL export binding (LoadLibrary /
> GetProcAddress), since svnserve is not even supported on Win 9x, as per
> SVN docs.
>
Oh, that's right. Then you indeed don't have to do anything; anyone who
tries running svnserve on Win9x/Me from now on will get an error about
unresolved symbols. That's fine.
>> 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.
>>
>
> The function is in main.c intentionally, because it is supposed to do
> whatever
> svnserve-specific logic for shutting down the service. Did you read the
> large block of comments in winservice.c that describe exactly this?
>
> winservice.c contains logic that is almost entirely service-specific, and
> does not contain anything specific to svnserve. svn_notify_stop() *is*
> svnserve-specific, and so lives in main.c. It is there in order to have
> access to the variables that main() uses.
>
I was in fact uneasy seeing all those Windows API calls in a file that
should ideally use only platform-independent stuff (i.e., APR). But I
suppose that later on we can extend this function to DTRT on other
platforms, too: for example, we could catch KILL signals on Unix in
order to cleanly stop the daemon.
>
>> Define such constants unconditionally.
>>
>
> Ok.
>
>
>> This is where you should use apr_os_sock_get.
>>
>
> Ok.
>
>
>> 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.
>>
>
> I did my absolute best to reduce the windows-specific modifications
> ("clutter") in main.c, which should be obvious from an unjaundiced
> reading of the patch. This really seems like over-the-top nitpicking.
>
Where did you get the idea that I'm opposed to your patch?
As for nitpicking, If I were implementing your design, I'd have done
exactly as I suggested. Code readability is important, and there are
already many #ifdefs in main.c as it is. Reducing them in a way that
doesn't obfuscate the code is goodness.
> Considering Unix does not *have* a service manager, there is no
> such thing as a non-Windows version of winservice_is_stopping().
>
If winservice_is_stopping() always returns FALSE on Unix, then it's not
lying, is it?
Which reminds me of something I seem to have overlooked;
winservice_is_stopping returns a boolean, so it should be declared as
svn_boolean_t, not int.
>> 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.
>>
>
> Ok, I'll change this. Is "ho hum" necessary? It's patronizing.
>
Just thinking out loud, old boy. You'll get used to me. :)
(And I meant "includes", not "defines", sorry.)
>> 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]
>>
>
> Why do you object to a few sentences that may help a future developer?
> It's relevant information that a future developer might find helpful.
>
I don't object to relevant information. I do object to _duplicated_
information, because it tends to get out of sync fairly quickly.
> Do you have a justification for your hatred of diagnostic information, or
> will SVN_DEBUG stay your wrath?
You're really touchy, aren't you. :)
I do not "hate" diagnostic information, or am I angry about anything.
Perhaps I should have said that I'd rather see a generic logging
infrastructure in svnserve than a service-specific one.
I also have very strong opinions about the *quality* of diagnostic info;
too many times I've seen programs report irrelevant stuff that only
obscured what what was actually going on. That's what I was getting at
when I mentioned consistency in the content.
As for SVN_DEBUG, it was introduced for such situations. For example,
elsewhere it controls if we include location information within
svn_error_t's.
> Do you even know what OutputDebugString does?
>
Oh well, I've only been hacking Windows since the 3.1 days, so I may
have forgotten a few details here and there ... ;)
>> 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.
>>
>
> Which would be a waste of heap / pool headers, and would also provide
> an unnecessary failure path. Why such antipathy for a global variable?
> The purpose of these variables is truly global. Global data is NOT evil,
> although many programs have abused it. Its use here is safe. If you
> can see a problem with it, other than "I don't like icky global data", then
> point it out, and I'll fix it.
>
Again, thinking out loud.
>> We use WIN32, not _WIN32, and this define is obviously not necessary.
>>
>
> Ok. It was there to give developers the option of disabling the service
> support, even on Windows, if they wanted to.
>
Don't we already have a switch for that? If you run svnserve without
--service, it'll behave exactly as it does now.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: [PATCH] Windows service support
Posted by Arlie Davis <ad...@stonestreetone.com>.
> 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.
Already fixed.
> Hm, I think parts of this should ideally go into INSTALL.
> Or at least be linked from there.
Agreed.
> + 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?
I don't know if SC.EXE gets this right -- I kind of doubt it. I know for a
fact that Windows *itself* supports quoted names in the binary path.
So it's just a matter of creating the service with the right path. I'll
check
and see if I can convince SC.EXE to create a service with quotes in the
binary path.
Earlier, during discussion, it was agreed that the best path was to get
this basic support committed first, and then deal with installation
applications. SC.EXE is just a crude "developer" interface to the service
control APIs, and is not intended to be the "real" interface. I already
said
that I will provide a service installer -- in a future patch. For now, we
can
document the fact that SC.EXE probably gets confused with double-quotes
in the binary path.
As such, this criticism (though fair) applies to service installers, not to
this patch, which deals solely with service support in svnserve itself.
[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. :)
Please don't pretend or imply that Windows "sysadmins" are not
"real" system administrators, or are incompetent or are stupid. It's
offensive and patronizing. I've worked in both environments, and I've
met plenty of Unix idiots, too.
The same instructions should be part of any installation document,
whether it targets Unix "sysadmins" or Windows "sysadmins".
> Um. No. The contact is users@subversion.tigris.org.
> We don't have feature maintainers on this project.
Ok.
> 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.
Ok.
> I get the feeling that you didn't read hacking.html very carefully.
Any first-time contributor is bound to miss things. I will learn,
as all have.
> 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.
That won't work. The service-specific APIs are not even present on Win 9x.
I would rather not have to use run-time DLL export binding (LoadLibrary /
GetProcAddress), since svnserve is not even supported on Win 9x, as per
SVN docs.
> +static SOCKET winservice_accept_socket_handle;
>
> Nope. See apr_os_socket_t and apr_os_sock_get in apr_portable.h.
Ok.
> 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.
The function is in main.c intentionally, because it is supposed to do
whatever
svnserve-specific logic for shutting down the service. Did you read the
large block of comments in winservice.c that describe exactly this?
winservice.c contains logic that is almost entirely service-specific, and
does not contain anything specific to svnserve. svn_notify_stop() *is*
svnserve-specific, and so lives in main.c. It is there in order to have
access to the variables that main() uses.
> Define such constants unconditionally.
Ok.
> This is where you should use apr_os_sock_get.
Ok.
> 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.
I did my absolute best to reduce the windows-specific modifications
("clutter") in main.c, which should be obvious from an unjaundiced
reading of the patch. This really seems like over-the-top nitpicking.
Considering Unix does not *have* a service manager, there is no
such thing as a non-Windows version of winservice_is_stopping().
> Just call it winservice.c, without the svn_ prefix.
Already changed.
> 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.
Ok, I'll change this. Is "ho hum" necessary? It's patronizing.
> 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.
Ok.
> 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]
Why do you object to a few sentences that may help a future developer?
It's relevant information that a future developer might find helpful.
> As I said before, we do not put author names in the source files. We
> have "svn log" for that.
Ok.
> If this is just a placeholder, why mark it for translation?
I didn't know that _ was used to mark for translation. I will change this.
> 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.
I'm not. They're necessary, their nature is truly global, their use is
constrained to a single, well-documented module, and their semantics
(usage) are correct.
> Please, no on-stack, fixed-size buffers in our code.
Ok.
> Buffer overrun. If the output of _vsnprintf would be longer than the
> buffer, it doesn't write a terminating '\0'.
I'll just eliminate dbg_print anyway. It was just there for debugging.
I didn't realize that _vsnprintf still did the wrong thing with buffer
termination.
> Sorry? Why are you doing this? Don't you have complete control over what
> goes in?
This code was there to assist me during developing and testing. It's
purpose is quite simple -- I don't have to worry about whether a dbg_print
format string was terminated (\n or \r\n) or not. I spend my life
developing
on a variety of platforms, and when I can, I prefer not to have to worry
about
whether the format string for debug prints needs a \r\n, a \n, or nothing
at all. As I've said, its presence will be subject to SVN_DEBUG, or
removed entirely.
> 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 was not aware of SVN_DEBUG, and I welcome its use.
Do you have a justification for your hatred of diagnostic information, or
will SVN_DEBUG stay your wrath? Do you even know what
OutputDebugString does?
> I also wish the _contents_ of the debug prints were more consistent.
Excuse me? They're debug prints. They're for developers, and their meaning
seems patently, obviously clear. Can you point out how they could be more
clear? And they are only sent to debuggers -- never to stdout / stderr.
As I said, I will make these subject to SVN_DEBUG, or simply remove them.
> 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.
Which would be a waste of heap / pool headers, and would also provide
an unnecessary failure path. Why such antipathy for a global variable?
The purpose of these variables is truly global. Global data is NOT evil,
although many programs have abused it. Its use here is safe. If you
can see a problem with it, other than "I don't like icky global data", then
point it out, and I'll fix it.
> Why do you close the handle, then call winservice_cleanup which will do
> that anyway?
> THere are several instances of this pattern.
Because the start event is not needed while the service is running. It is
only needed while the service is starting. winservice_cleanup runs when
the process is torn down. I'm releasing resources after they are no longer
needed.
> We use WIN32, not _WIN32, and this define is obviously not necessary.
Ok. It was there to give developers the option of disabling the service
support, even on Windows, if they wanted to.
> No, not extern. They're just prototypes.
Fine.
> 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!
My, how a single prototype inflames. It is obviously local to the
implementation, and during the process of debugging, I had need for
dbg_print in main.c. Foolishly, I left a harmless prototype in a header
file.
I will remove it.
---------------------------------------------------------------------
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>.
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
[PATCH] Next rev of Windows service support (Feb 27)
Posted by Arlie Davis <ad...@stonestreetone.com>.
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] Windows service support
Posted by Branko Čibej <br...@xbc.nu>.
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