> 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
Received on Tue Feb 28 17:33:34 2006