Re: winetests: new addition

[Ferenc Wagner]

"Dimitrie O. Paun"  writes:
> I'm still not sure how to support building with PE .exes

Could you outline the problems you can see?  Since it is
still not in the tree, instead of patches I send comments
and questions.
> +#include <shellapi.h>
> +#include <shellapi.h>

Do we really need it twice?
> - 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?
The size=min(...) does not make sense for me either.
va_start and va_end could be moved out of the loop by using
break instead of return.
> +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.
> - }
> - fprintf(fout, "    dwMajorVersion=%ld\n    dwMajorVersion=%ld\n"

The second is dwMinorVersion.
> +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?
> +int count_tests()

This is superfluous, not used for anything particularly
useful.  The progress bar still did not make it...
> - if((fout = fopen(test->exename, "wb")))
> - {
> -
>    fwrite(code, size, 1, fout);
> -
>    fclose(fout);
> - }

No error checking whatsoever...
> - char line[512], *cmd;

Fixed size buffers are wrong...
> - 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?
> - if (!(fp = fopen( logfile, "w" ))) fatal("Could not open log file.");

Why not append mode?  That would also make the above
redirection 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.
> +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...

So I am back, and ready to go for finishing this project...

Feri.