Re: svn commit: r23239 - trunk/subversion/libsvn_subr

Lieven,

Nice work! Some comments below:

On Thu, Jan 25, 2007 at 12:39:34PM -0800, lgo@tigris.org wrote:
> Added: trunk/subversion/libsvn_subr/win32_crashrpt.c
> +/* write string to file, allow printf style formatting */
> +static void
> +write_to_file(FILE *file, const char *format, ...)
> +{
> + va_list argptr;
> +
> + va_start(argptr, format);
> + vfprintf(file, format, argptr);
> + va_end(argptr);
> +}
> +

How is this not fprintf()?

> +static char *
> +convert_wbcs_to_utf8(const wchar_t *str)
> +{
> + size_t len = wcslen(str);
> + char *utf8_str = malloc(sizeof(wchar_t) * len + 1);

I'm assuming that STR here is the normal Windows 'wide' (really UTF-16) string,
and that it's 16-bit. In any case, even if the source is UCS-2, how
do you figure that the UTF-8 encoded version of the string will take up
the same space as as the original?

> +/* Convert the exception code to a string */
> +static const char *
> +exception_string(int exception)
> +{
> + #define EXCEPTION(x) case EXCEPTION_##x: return (#x);
> +
> + switch (exception)
> + {
> ...
> + }

Would be nice to #undef EXCEPTION here, to make it clear what the scope
ends.

> + write_to_file(log_file,
> + "eax=%08x ebx=%08x ecx=%08x edx=%08x esi=%08x edi=%08x\n",
> + context->Eax, context->Ebx, context->Ecx,
> + context->Edx, context->Esi, context->Edi);

What happens with x64 or ia64 executables, incidentally?

> +write_var_values(PSYMBOL_INFO sym_info, ULONG sym_size, void *baton)
> +{
> + char value_str[256] = "";
> +
> ...
> + format_value((char *)value_str, sym_info->ModBase, sym_info->TypeIndex,
> + (void *)var_data);

There's no need to cast value_str (occurs twice in this function).

> +/* write the details of one function to the log file */
> +static void
> +write_function_detail(STACKFRAME stack_frame, void *data)
> +{
> ...
> + write_to_file(log_file, ")", 1);

1?

> +/* walk over the stack and log all relevant information to the log file */
> +static void
> +write_stacktrace(CONTEXT *context, FILE *log_file)
> +{
> +#if defined (_M_IX86)
> + machine = IMAGE_FILE_MACHINE_I386 ;
> + stack_frame.AddrPC.Offset = context->Eip;
> + stack_frame.AddrStack.Offset = context->Esp;
> + stack_frame.AddrFrame.Offset = context->Ebp;
> +#elif defined (_M_AMD64)
> + machine = IMAGE_FILE_MACHINE_AMD64 ;
> + stack_frame.AddrPC.Offset = context->Rip;
> + stack_frame.AddrStack.Offset = context->Rsp;
> + stack_frame.AddrFrame.Offset = context->Rbp;
> +#endif

Is IA64 covered by the first case, or should we have an

#else
#error We don't know what to do!
#endif

> +/* Create a filename based on a prefix, the timestamp and an extension.
> + check if the filename was already taken, retry 3 times. */
> +BOOL
> +get_temp_filename(char *filename, const char *prefix, const char *ext)
> +{
> + char temp_dir[MAX_PATH - 14];
> + int i;
> +
> + if (! GetTempPathA(MAX_PATH - 14, temp_dir))
> + return FALSE;
> +
> + file = CreateFile(filename, GENERIC_WRITE, 0, NULL, CREATE_NEW,
> + FILE_ATTRIBUTE_NORMAL, NULL);

Are you intending for this code to work when compiled with UNICODE
defined? If yes, you just called CreateFileW with an ANSI filename. If
no, why do you call GetTempPathA explicitly? (There are also other
instances of this elsewhere, references to TCHAR etc).

> +LONG WINAPI svn_unhandled_exception_filter(PEXCEPTION_POINTERS ptrs);
> +

Shouldn't this be a private symbol? (svn__unhandled_exception_filter?)

I don't believe we're intending for users of libsvn_subr to use it
directly - it's only exported so that cmdline.c can see it, and it
doesn't seem useful for non-command-line programs.

Finally, is it okay to assume that all command-line programs will be
happy for us to add in our own exception handler? Seems a bit rude for
a library to just assume that, though I doubt there's any real harm.

Regards,
Malcolm