> 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
Received on Sat Feb 25 03:54:39 2006