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
Received on Sat Feb 25 14:01:09 2006