Re: winetests: new addition

[Dimitrie O. Paun]

On October 18, 2003 07:04 am, Ferenc Wagner wrote:
> > +#include <shellapi.h>
> > +#include <shellapi.h>
>
> Do we really need it twice?

Of course not :)
> > - while (1)
> > - {
> > -
> >    va_start(ap, fmt);
> > - n = vsnprintf (p, size, fmt, ap);
> > - va_end(ap);
> > -
> >    if (n > -1 && n < size) return p;
> > - size = min( size*2, n+1 );
> > - if(!(p = realloc (p, size))) fatal("Out of memory.");
> > - }
>
> MSDN has no vnsprintf() only _vsnprintf(), what is this?

It should have a vnsprintf() into some import lib oldnames IIRC.
> The size=min(...) does not make sense for me either.

Hmmm, I can't remember why the min either. It should
be more like so:
    size = (n < 0) ? size*2 : n + 1;
> va_start and va_end could be moved out of the loop by using
> break instead of return.

This was just copied from tools/winegcc/utils.c. Feel free to
improve it, but please fix it in both places.
> > +int print_version(FILE *fout)
> > +{
> > - OSVERSIONINFOEX ver;
> > -
> > - ver.dwOSVersionInfoSize = sizeof(OSVERSIONINFOEX);
> > - if (!GetVersionEx ((OSVERSIONINFO *) &ver))
> > - {
> > - ver.dwOSVersionInfoSize = sizeof(OSVERSIONINFO);
> > - if (!GetVersionEx ((OSVERSIONINFO *) &ver))
> > -
> >    return 0;
>
> This should be made a fatal(), I think.

Indeed.
> > - }
> > - fprintf(fout, "    dwMajorVersion=%ld\n    dwMajorVersion=%ld\n"
>
> The second is dwMinorVersion.

Yes, copy&paste error.
> > +void deletePath( char *path )
> > +{
> > - SHFILEOPSTRUCT fileop;
> > -
> > - path[strlen( path ) + 1] = '\0';
>
> We do not have space allocated for this NUL.
>
> Here we use the advanced Shell API but later not the Win32
> API for opening files and such.  What is our policy?

No policy. Jakob wrote this one, and I just used it.
If it were the only thing imported from shell32, I'd
get rid of it, but we also need the ShellExecute().
> > +int count_tests()
>
> This is superfluous, not used for anything particularly
> useful.  The progress bar still did not make it...

Right, but I hope we'll have one eventually.
> > - if((fout = fopen(test->exename, "wb")))
> > - {
> > -
> >    fwrite(code, size, 1, fout);
> > -
> >    fclose(fout);
> > - }
>
> No error checking whatsoever...

Yeah, I was lazy, sue me :P
> > - char line[512], *cmd;
>
> Fixed size buffers are wrong...

Oh come on, it's just a temp buffer to copy stuff through.
It introduces no limitation.
> > - if ((fp = popen( cmd, "r" )))
> > - {
> > - while (fgets( line, sizeof(line), fp ))
> > -
> >    fprintf( logfp, "%s", line );
> > - fclose( fp );
> > - }
>
> Why not a CreateProcess with redirected stderr/stdout?

Sure.
> > - if (!(fp = fopen( logfile, "w" ))) fatal("Could not open log
> >
> > file.");
>
> Why not append mode?  That would also make the above
> redirection more secure.

Why append mode? How is it making it more secure?
> > - for(test = wine_tests; test->name; test++)
>
> The number of tests could be determined by sizeof instead of
> a special member, but I am not sure that would be worth it.

I know, it was simple/cleaner to write the loop like so.
> > +BINDIR="../.."
>
> [...]
> > +for test in $TEST_EXES; do
> > - testname=`basename $test .exe`
> > - filename="$BINDIR/$test.so"
>
> I am probably too tired, but can not see how you find the
> test .so-s in the dlls directory...

I don't find anything. TEST_EXES is a list of tests, stuff
that looks like this:
   dlls/user/tests/user32.exe
   dlls/gdi/tests/gdi32.exe
   ... etc ...

BINDIR is points to the root of your build dir, something
like /home/dimi/dev/wine/wine.build

This bit works, did I miss anything?
> So I am back, and ready to go for finishing this project...

Cool, feel free to change the program as you wish.