On 1/23/07, Lieven Govaerts <svnlgo@mobsol.be> wrote:
> Ivan, DJ,
>
> thanks for your review. I think your remarks (and some others) into
> account for the next version of the patch.
Lieven, I forgot about new version of your patch. Excuse me.
>
> Ivan Zhakov wrote:
> > On 1/3/07, Lieven Govaerts <lgo@mobsol.be> wrote:
> >>
> >> Attached is a patch that implements a crash handler (Windows specific)
> >> for the command line apps. While I learned a lot from Stefan Küng's
> >> patch posted here some time ago, it's basically a new implementation.
> >> Review of my choices (see below) and the patch itself are greatly
> >> appreciated.
> >>
> > Looks very nice! One thing that I didn't understand from this
> > description is where are minudump and log files get written?
> The log and minidump files are written in the temp folder. I prefer the
> temp folder over Subversion config folder, because these files don't
> have to be stored on the user's pc forever.
Ok, sounds reasonable.
>
> >> + 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.
> >> + size_t len = wcslen(str);
> >> + char *utf8_str = malloc(sizeof(wchar_t) * len + 1);
> >> + len = wcstombs(utf8_str, str, len);
> >> + utf8_str[len] = '\0';
> > wcstombs can return -1 in some situation:
> > http://msdn.microsoft.com/library/default.asp?url=/library/en-us/vccore98/HTML/_crt_wcstombs.asp
> >
> Ah ok, this situation is now handled.
[snip
> > many unbuffered writes to file. I suggest create file use run-time
> > function open(), then get Windows handle using _get_osfhandle() for
> > MiniDumpWrite.
> > And use standard stream API in this function.
> Writing to the log file is now buffered. I don't see how your suggestion
> works for MiniDumpWrite, you're basically getting the same handle as
> what you'd get using CreateFile no?
Sorry, I was thinking that you writing log to the same file as dump.
Just forget about my comment.
> > 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.
> >> I think it's better to check for NULL procedure address in one place.
> > I mean just:
> > if (!SymFromAddr_ ||
> > !SymGetModuleBase_ ||
> > ! ....)
> > goto cleanup;
> Fixed.
> >> + if (! GetTempPathA(MAX_PATH - 14, temp_dir))
> >> + return;
> >> +
> >> + for (i = 0;i < 3;i++)
> > Why try 3 times? What is the reason?
> If the file already exists (unlikely though as it includes the
> timestamp), we just try again with a new timestamp.
> >> + {
> >> + HANDLE file;
> >> + time_t now;
> >> + char time_str[64];
> >> +
> >> + time(&now);
> >> + strftime(time_str, 64, "%Y%m%d%H%M%S", localtime(&now));
> >> + sprintf(filename, "%s%s%s.%s", temp_dir, prefix, time_str, ext);
> >> +
> >> + 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.
> >> + get_temp_filename(dmp_filename, "svn-crash-log", "dmp");
> >> + get_temp_filename(log_filename, "svn-crash-log", "log");
> > Personally I prefer Application Data\Subversion for creating dump files.
> Why?
Just easier to find than temporary files. Anyway temporary directory
is ok for me.
--
Ivan Zhakov
---------------------------------------------------------------------
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:31:41 2007