Re: [PATCH] Next rev of Windows service support (Feb 27)

> > +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
Received on Tue Feb 28 17:42:04 2006