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.

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.

winetests: new addition

[Ferenc Wagner]

"Dimitrie O. Paun"  writes:
> On October 18, 2003 07:04 am, Ferenc Wagner wrote:
> > > - 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.

Sorry, I was too quick here.
> > > - while (fgets( line, sizeof(line), fp ))
> > > -
> > >    fprintf( logfp, "%s", line );

But here is the reason: I was sure fgets was deprecated.
For whole bunch of libc input functions this is for possible
buffer overruns, but not for fgets.  The problem here is
that you can not tell a NUL in the input stream.
> > > - 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?

In principle, if something went wrong in a test, it could
seek back in its output stream.  We are only appending anyway.
> This bit [BINDIR] works, did I miss anything?

No, I was confused.  No wonder, I am not at all familiar
with the build system...

Have you got an idea why your patch has not been committed?

Feri.

winetests: new addition

[Dimitrie O. Paun]

On October 19, 2003 01:05 pm, Ferenc Wagner wrote:
> But here is the reason: I was sure fgets was deprecated.
> For whole bunch of libc input functions this is for possible
> buffer overruns, but not for fgets.  The problem here is
> that you can not tell a NUL in the input stream.

Yes, I know, I don't think it's terribly important here,
feel free to change it if you wish. However, I think we'd
better stick to the standard C functions for file IO, we
might not want to make this program dependent on wine on
Linux (why, I don't know :)).
> Have you got an idea why your patch has not been committed?

Not really. Maybe because I haven't solved the cross compilation
problem... Alexandre was rather busy with the DLL separation
work, I didn't want to bother him with this stuff. But now that
he finished... <g>

winetests: new addition

[Ferenc Wagner]

"Dimitrie O. Paun"  writes:
> On October 19, 2003 01:05 pm, Ferenc Wagner wrote:
> > Have you got an idea why your patch has not been committed?
>
> Not really. Maybe because I haven't solved the cross compilation
> problem... Alexandre was rather busy with the DLL separation
> work, I didn't want to bother him with this stuff. But now that
> he finished... <g>

It is a shame, but I can not apply your patch...  It gives:

$ patch -p0 <~winetest.patch
patching file configure.ac
Hunk #1 succeeded at 1580 (offset 3 lines).
patching file programs/winetests/Makefile.in
patch: **** malformed patch at line 48: --- /dev/null   7 Oct 2003 05:24:37 -0500

Line 48 is the second --- /dev/null line.  As you can see, I
tried to tweak the date format, insert empty lines etc., but
nothing helped.  I really feel stupid...

Feri.

winetests: new addition

[Dimitrie O. Paun]

On Tue, 21 Oct 2003, Ferenc Wagner wrote:
> It is a shame, but I can not apply your patch...  It gives:

Sorry about that, maybe I should submit a new patch. I'll
do so when I get home, meanwhile here is a tar.gz of the
new files.

winetests: new addition

[Ferenc Wagner]

"Dimitrie O. Paun"  writes:
> here is a tar.gz of the new files.

Thanks, that is fine.  I came across a webpage
http://www.websteves.com/win32asm/datrsrc.htm
stating that DISCARDABLE, LOADONCALL etc. are
not needed anymore.  Do we still need them?

Feri.

winetests: new addition

[Dimitrie O. Paun]

On October 21, 2003 07:43 pm, Ferenc Wagner wrote:
> Thanks, that is fine.  I came across a webpage
> http://www.websteves.com/win32asm/datrsrc.htm
> stating that DISCARDABLE, LOADONCALL etc. are
> not needed anymore.  Do we still need them?

Good point, I'll get rid of them.

winetests: new addition

[Jakob Eriksson]

Dimitrie O. Paun wrote:
> > > +void deletePath( char *path )
> > > +{
> > > - SHFILEOPSTRUCT fileop;
> > > -
> > > - path[strlen( path ) + 1] = '\0';
> >
> > We do not have space allocated for this NUL.

Se later down this message...
> > 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().

My thought exactly.  I also tried to write a recursive delete using 
plain Win32 but
failed miserably. :-)
> > > - 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.

Yep.  Same thing with MAXPATH.  I think I set some buffers
to MAXPATH + 2 or something. Hence the extra null is accounted for.

(Why is the code not in CVS BTW? I don't know where to look.)
> > Why not append mode?  That would also make the above
> > redirection more secure.
>
> Why append mode? How is it making it more secure?

I tried append mode.  That screwed the file up totally when tests crashed
on Win98.  So I changed to "w". I don't know if this applies at all to how
Dimi rewrote the program.

regards,
Jakob

winetests: new addition

[Ferenc Wagner]

Hi there,

below is my present vision of main.c.  It does not work:
seems like the spawnvp call does nothing.  Can you give me a
clue?
                                        Feri.

/*
 * Wine Conformance Test EXE
 *
 * Copyright 2003 Jakob Eriksson   (for Solid Form Sweden AB)
 * Copyright 2003 Dimitrie O. Paun
 *
 * This library is free software; you can redistribute it and/or
 * modify it under the terms of the GNU Lesser General Public
 * License as published by the Free Software Foundation; either
 * version 2.1 of the License, or (at your option) any later version.
 *
 * This library is distributed in the hope that it will be useful,
 * but WITHOUT ANY WARRANTY; without even the implied warranty of
 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
 * Lesser General Public License for more details.
 *
 * You should have received a copy of the GNU Lesser General Public
 * License along with this library; if not, write to the Free Software
 *
 * This program is dedicated to Anna Lindh,
 * Swedish Minister of Foreign Affairs.
 * Anna was murdered September 11, 2003.
 *
 */

#include "config.h"
#include "wine/port.h"

#include <stdio.h>
#include <stdlib.h>
#include <errno.h>
#include <ctype.h>
#include <string.h>
#include <unistd.h>
#include <sys/stat.h>

#include <wtypes.h>
#include <shellapi.h>

#include "winetests.h"

void fatal(const char* msg)     /* Funny when out of memory... */
{
    MessageBox( NULL, msg, "Fatal Error", MB_ICONERROR | MB_OK );
    exit(1);
}

void *xmalloc (size_t len)
{
    void *p;

p = malloc (len);
    if (!p) fatal ("Out of memory.");
    return p;
}

void xprintf (const char *fmt, ...)
{
    va_list ap;

va_start (ap, fmt);
    if (vprintf (fmt, ap) < 0) fatal ("Can't write logs.");
    va_end (ap);
}

char *strmake(const char *fmt, ...)
{
    int n;
    size_t size = 100;
    char *p = xmalloc (size);
    va_list ap;

va_start(ap, fmt);
    while (1)
    {
    n = vsnprintf (p, size, fmt, ap);
        if (n > -1 && n < size) break;
    size = (n < 0) ? size*2 : n + 1;
    if(!(p = realloc (p, size))) fatal("Out of memory.");
    }
    va_end(ap);
    return p;
}

void print_version ()
{
    OSVERSIONINFOEX ver;
    BOOL ext;

ver.dwOSVersionInfoSize = sizeof(OSVERSIONINFOEX);
    if (!(ext = GetVersionEx ((OSVERSIONINFO *) &ver)))
    {
    ver.dwOSVersionInfoSize = sizeof(OSVERSIONINFO);
    if (!GetVersionEx ((OSVERSIONINFO *) &ver))
        fatal("Can't get OS version.");
    }

xprintf ("    dwMajorVersion=%ld\n    dwMinorVersion=%ld\n"
             "    dwBuildNumber=%ld\n    PlatformId=%ld\n    szCSDVersion=%s\n",
             ver.dwMajorVersion, ver.dwMinorVersion, ver.dwBuildNumber,
             ver.dwPlatformId, ver.szCSDVersion);

if (ext)
        xprintf ("    wServicePackMajor=%d\n    wServicePackMinor=%d\n"
                 "    wSuiteMask=%d\n    wProductType=%d\n    wReserved=%d\n",
                 ver.wServicePackMajor, ver.wServicePackMinor, ver.wSuiteMask,
                 ver.wProductType, ver.wReserved);
}

void deletePath( const char *path )
{
    SHFILEOPSTRUCT fileop;
    size_t len = strlen (path);
    char *paths = xmalloc (len+1);

memcpy (paths, path, len);
    paths[len] = 0;

fileop.hwnd = NULL;
    fileop.wFunc = FO_DELETE;
    fileop.pFrom = paths;
    fileop.pTo = NULL;
    fileop.fFlags = FOF_NOCONFIRMATION | FOF_NOERRORUI;

if (SHFileOperation( &fileop )) /* FIXME not present in NT3 */
        fatal (strmake ("Can't delete temporary directory: %s.", path));
}

int count_tests()
{
    struct wine_test* test;
    int total = 0;

for (test = wine_tests; test->name; test++)
    total += test->subtest_count;

return total;
}

LPVOID extract_rcdata (const char *name, DWORD* size)
{
    HRSRC rsrc;
    HGLOBAL hdl;

if (!(rsrc = FindResource( 0, name, RT_RCDATA ))) return 0;
    if (!(*size = SizeofResource( 0, rsrc ))) return 0;
    if (!(hdl = LoadResource( 0, rsrc ))) return 0;
    return LockResource(hdl);
}

void extract_test (const char *dir, struct wine_test* test)
{
    BYTE* code;
    DWORD size;
    FILE* fout;

if (!(code = extract_rcdata(test->name, &size)))
      fatal (strmake ("Can't get resource %s.", test->name));

test->is_elf = (code[1] == 'E' && code[2] == 'L' && code[3] == 'F');
    test->exename = strmake("%s/%s.exe%s", dir, test->name,
    		    test->is_elf ? ".so" : "");

if (!(fout = fopen(test->exename, "wb")) ||
        (fwrite (code, size, 1, fout) != 1) ||
        fclose (fout)) fatal (strmake("Failed to write file %s.", test->name));
}

void run_test (struct wine_test* test, const char* subtest)
{
    int status;
    const char *argv[]={test->exename, subtest, NULL};

xprintf ("%s:%s start\n", test->name, subtest);
    status = spawnvp (_P_WAIT, test->exename, argv);
    if (status)
        xprintf ("Exit status: %d, errno=%d: %m\n", status, errno);
    xprintf ("%s:%s done\n", test->name, subtest);
}

int WINAPI WinMain( HINSTANCE hInst, HINSTANCE hPrevInst, LPSTR cmdline, int cmdshow )
{
    struct wine_test* test;
    int nr_of_tests, subtest;
    char *tempdir, *logfile;
    FILE *fp;

SetErrorMode( SEM_FAILCRITICALERRORS );

if (!(tempdir = tempnam(0, "wct"))) fatal("Can't name temporary dir (check TMP).");
    fprintf (stderr, "tempdir=%s\n", tempdir);
    if (mkdir( tempdir, 0777 )) fatal(strmake("Could not create directory: %s", tempdir));
    if (chdir( tempdir )) fatal(strmake("Could not make %s current directory.", tempdir));

if (!(logfile = tempnam(0, "res"))) fatal("Can't name log file.");
    fprintf (stderr, "logfile=%s\n", logfile);
    if (!(fp = fopen( logfile, "a" ))) fatal("Could not open log file.");
    if (-1 == dup2 (fileno (fp), 1)) fatal ("Can't redirect stdout.");

xprintf ("Tests from build %s\n", "FIXME" );
    xprintf ("Operating system version:\n");
    print_version ();
    xprintf ("Test output:\n" );

for (test = wine_tests; test->name; test++)
        extract_test (tempdir, test);

nr_of_tests = count_tests();

for (test = wine_tests; test->name; test++)
    for (subtest = 0; subtest < test->subtest_count; subtest++)
        run_test (test, test->subtests[subtest]);

fflush (stdout);
    fclose (fp);

deletePath( tempdir );

return 0;
}

winetests: new addition

[Jakob Eriksson]

Ferenc Wagner wrote:
> Here we use the advanced Shell API but later not the Win32
> API for opening files and such.  What is our policy?

Aha... now I understand what you mean. The fopen instead of some Win32-ism.
Also inherited from my version of the program, no doubt.  I just used 
cause I was
familiar with it. I guess it would be looking to use some Win32 - stuff.