SteveKing wrote:
>Index: subversion/libsvn_subr/cmdline.c
>===================================================================
>--- subversion/libsvn_subr/cmdline.c (Revision 15391)
>+++ subversion/libsvn_subr/cmdline.c (Arbeitskopie)
>@@ -36,6 +36,7 @@
> #include "svn_pools.h"
> #include "svn_error.h"
> #include "utf_impl.h"
>+#include "svn_version.h"
>
> #include "svn_private_config.h"
>
>@@ -56,13 +57,50 @@
> /* The stdout encoding. If null, it's the same as the native encoding. */
> static const char *output_encoding = NULL;
>
>+#ifdef WIN32
>+/* Client crash callback */
>+typedef BOOL (CALLBACK *LPGETLOGFILE) (LPVOID lpvState);
>+/* Stack trace callback */
>+typedef void (*TraceCallbackFunction)(DWORD address, const char *ImageName,
>+ const char *FunctionName, DWORD functionDisp,
>+ const char *Filename, DWORD LineNumber, DWORD lineDisp,
>+ void *data);
>
>
A general not on style: We don't allow tabs or lines longer than 80 columns.
>+typedef LPVOID (*InstallEx)(LPGETLOGFILE pfn, LPCSTR lpcszTo, LPCSTR lpcszSubject);
>+typedef void (*UninstallEx)(LPVOID lpState);
>+HMODULE crashDll = 0;
>+LPVOID lpvState = 0;
>+svn_cmdline_remove_crashrpt()
>+{
>+ UninstallEx pfnUninstallEx;
>+ if ((crashDll)&&(lpvState))
>+ {
>+ pfnUninstallEx = (UninstallEx)GetProcAddress(crashDll, "UninstallEx");
>+ (UninstallEx)(lpvState);
>+ }
>+ FreeLibrary(crashDll);
>+}
>+#endif
>
>
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.
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.
You're not checking the return value of GetProcAddress. It _can_ fail,
and if it does, the client shouldn't crash.
> 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 pfnInstallEx;
>+ crashDll = LoadLibrary("CrashRpt");
>+ if (crashDll)
>+ {
>+ pfnInstallEx = (InstallEx)GetProcAddress(crashDll, "InstallEx");
>+ lpvState = (pfnInstallEx)(NULL, "crashreports@subversion.tigris.org", SVN_VER_NUM);
>+ }
>+ atexit(svn_cmdline_remove_crashrpt);
>+#endif
>+
>
>
Most of the above comments apply here, too.
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.
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.
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.
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).
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sun Jul 24 14:58:16 2005