Branko Čibej wrote:
> A general not on style: We don't allow tabs or lines longer than 80
Sorry about that. I've now configured my editor to show a line at the 80
> First of all, svn_cmdline_remove_crashrpt should not be a public
> function. It should be static, and not have the svn_ prefix in the name.
> The way you use crashDll and lpvState is not thread safe. You can assume
> that svn_cmdline_init is called in a single-threaded environment, but
> I'm not sure about the teardown functions. Presumably atexit will be
> called once, but we hav to be sure about this.
> Next: you're missing a ton of docstrings here. Generally every variable
> and function should have a docstring.
> You're still using Hungarian notation even with symbols that are not
> related to the contents of crashrpt.dll. You're not conforming to the
> indentation rules, etc. etc.
Hope it's done now.
> You're not checking the return value of GetProcAddress. It _can_ fail,
> and if it does, the client shouldn't crash.
> One thing I'm nervous about is your calling GetProcAddress instead of
> GetProcAddressA, when the name of the procedure is always a
> narrow-character string.
There's no GetProcAddressA and no GetProcAddressW - just GetProcAddress
which takes a narrow char string. At least in the SDK I'm using here.
> Regarding the mailing address: I suppose the breakage address would be
> the most appropriate here (email@example.com), unless
> people thing we need a new mailing list for crash reports.
> I don't like the idea of command-line programs popping up dialogue
> boxes. This is especially annoying when the program in question is a
> server, such as svnserve. I'd like to have a way to configure the crash
> reporter to dump the data in a text file instead, and perhaps leave a
> pointer to that file in the event log.
That would require a change to the crashreporter dll itself.
Also, I don't quite see the point: if svnserve crashes, it won't restart
automatically. So having a dialog open which shows info about the crash
> Another issue is the name of crashrpt.dll. This seems to me to be a much
> too generic name, and likely to collide with other DLLs on the system.
> svn_crachrpt.dll would be much better. (If you'll recall, we had
> problems with intl.dll, which is why we renamed our copy to svn_intl3.dll).
Changed it to svn_CrashRpt.dll.
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.tigris.org
Load a crash reporting dll on Windows when clients start up.
load the crashrpt.dll dynamically
--- subversion/libsvn_subr/cmdline.c (Revision 15403)
+++ subversion/libsvn_subr/cmdline.c (Arbeitskopie)
@@ -37,6 +37,7 @@
@@ -49,13 +50,71 @@
/* The stdout encoding. If null, it's the same as the native encoding. */
static const char *output_encoding = NULL;
+/* typedef of the GetLogFile function used in the InstallEx function
+ as the first parameter. Even though we pass NULL for that parameter
+ later, the typedef is needed for type checks */
+typedef BOOL (__stdcall *CrashGetLogFile) (void * crashstate);
+/* the function to initialize the crash handler. The @a CrashGetLogFile function
+ can be used to add custom files to the crashreport. @a mailto must be set
+ to the mail address the report should be sent to, and @a mailsubject must
+ be set to the subject the mail should have. */
+typedef LPVOID (*InstallEx)(CrashGetLogFile func, const char * mailto,
+ const char * mailsubject);
+/* function to remove the custom crash handler. @a crashstate is the pointer
+ to the state variable received in the InstallEx call. */
+typedef void (*UninstallEx)(void * crashstate);
+/* the handle to the crash report dll */
+HMODULE crashDll = 0;
+/* the pointer to the crash report state */
+void * crashstate = 0;
+/* Removes the crash report handler from the system. After this call
+ exceptions are passed through to the system and not handled by the
+ custom dll anymore. */
+static void cmdline_remove_crashrpt(void)
+ UninstallEx UninstallFunc;
+ if ((crashDll)&&(crashstate))
+ UninstallFunc = (UninstallEx)GetProcAddress(crashDll, "UninstallEx");
+ if (UninstallFunc)
+ crashstate = 0;
+ crashDll = 0;
svn_cmdline_init (const char *progname, FILE *error_stream)
+ /* Load the crash reporting dll if it's available */
+ InstallEx InstallExFunc;
+ crashDll = LoadLibrary("svn_CrashRpt");
+ if (crashDll)
+ InstallExFunc = (InstallEx)GetProcAddress(crashDll, "InstallEx");
+ if (InstallExFunc)
+ crashstate = InstallExFunc(NULL,
struct stat st;
To unsubscribe, e-mail: firstname.lastname@example.org
For additional commands, e-mail: email@example.com
Received on Sun Jul 24 18:29:15 2005