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