Ivan Zhakov wrote:
>> >> + va_start(argptr, format);
>> >> + len = vsprintf(buf, format, argptr);
>> >> + va_end( argptr );
>> > Unbounded sprintf. At least your should use vsnprintf or create FILE
>> > against handle and use fprintf.
>> Yep, you're right. I've changed it to use the CRT file handling
>> functions, this also solves the buffering problem (if any, I'm not so
>> sure).
>
> Ok, but I see that you keep the function write_to_file which only
> forwards calls to fprintf. Why? You can just use fprintf for printing
> to file.
The original reason was to make it possible to change the log target,
like an XML file or an internal data structure, but that changed later
to directly writing to file, so there's indeed no real need anymore for
this function.
> > The name check_dll_version is little bit confusing. I suggest name
>> > check_dbghelp_version().
>> The function supports any dll, but I've replaced it with
>> check_dbghelp_version now.
>> > GetFileVersionInfoSize, GetFileVersionInfo and VerQueryValue are
>> > supported on all versions of Windows so there is no need to call them
>> > using GetProcAddress.
>> The reason I load them here is because I don't want to load any extra
>> dll at process startup (or initialisation of the crash handler in this
>> case).
> I don't have strong objection against this code, but for me it's
> overkill.
Ok, but do you agree that we shouldn't slow down start up of the svn
processes to load a dll which is only used in case the process crashes?
As far as I saw during my test sessions for this patch, this is the only
way to achieve this.
>>
>> >> + file = CreateFile(filename, GENERIC_WRITE, 0, NULL,
>> CREATE_NEW,
>> >> + FILE_ATTRIBUTE_NORMAL, NULL);
>> > It's better to use access() for testing file presence.
>> access()?
> Yes, access is posix function and it's supported on Windows.
I couldn't find it in msdn, do you have an online reference available?
Do you have an opinion on the message to the end-user? I think we should
the user tell explicitly not to send the minidump file to the mailing
list, but I'd like to know what others think about it.
The code is on trunk now so feel free to change it.
Lieven
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Jan 25 22:46:21 2007