[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH] Issue 1628

From: SteveKing <steveking_at_gmx.ch>
Date: 2005-07-24 18:28:23 CEST

Branko Čibej wrote:

> A general not on style: We don't allow tabs or lines longer than 80
> columns.

Sorry about that. I've now configured my editor to show a line at the 80
column.

> Ouch.
> 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.

Done.

> 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.

Done.

> Next: you're missing a ton of docstrings here. Generally every variable
> and function should have a docstring.

Done.

> 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.

Done.

> 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 (svn-breakage@subversion.tigris.org), unless
> people thing we need a new mailing list for crash reports.

Done.

> 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
won't hurt?

> 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.

Patch attached.

Stefan

-- 
        ___
   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.
* subversion/libsvn_subr/cmdline.c(svn_cmdline_init):
  load the crashrpt.dll dynamically
]]]
Index: subversion/libsvn_subr/cmdline.c
===================================================================
--- subversion/libsvn_subr/cmdline.c (Revision 15403)
+++ subversion/libsvn_subr/cmdline.c (Arbeitskopie)
@@ -37,6 +37,7 @@
 #include "svn_error.h"
 #include "svn_nls.h"
 #include "utf_impl.h"
+#include "svn_version.h"
 
 #include "svn_private_config.h"
 
@@ -49,13 +50,71 @@
 /* The stdout encoding. If null, it's the same as the native encoding. */
 static const char *output_encoding = NULL;
 
+#ifdef WIN32
+/* 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)
+ {
+ UninstallFunc(crashstate);
+ crashstate = 0;
+ }
+ }
+ FreeLibrary(crashDll);
+ crashDll = 0;
+}
+#endif
+
+
 int
 svn_cmdline_init (const char *progname, FILE *error_stream)
 {
   apr_status_t status;
   apr_pool_t *pool;
 
+ /* Load the crash reporting dll if it's available */
+#ifdef WIN32
+ InstallEx InstallExFunc;
+ crashDll = LoadLibrary("svn_CrashRpt");
+ if (crashDll)
+ {
+ InstallExFunc = (InstallEx)GetProcAddress(crashDll, "InstallEx");
+ if (InstallExFunc)
+ crashstate = InstallExFunc(NULL,
+ "svn-breakage@subversion.tigris.org",
+ SVN_VER_NUM);
+ }
+ atexit(cmdline_remove_crashrpt);
+#endif
+
 #ifndef WIN32
   {
     struct stat st;

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sun Jul 24 18:29:15 2005

This is an archived mail posted to the Subversion Dev mailing list.