Michael Karcher wrote:
Am Montag, den 13.10.2008, 21:05 -0400 schrieb Chris Ahrendt:
Fixed a trailing space error..
Sorry to chime in so late on this patchset.
First, some general remarks: The subject of your mail will be the one-line commit message. "Try #2" is not a good choice for that. Also it does not have enough context for the casual wine-patches reader, as the message overview does not show what you patch is about. The usual convention is to take the subject of the first try, and append (or prepend) "(try 2)" to that subject.
Subject: [PATCH] Pared Down Test For ddraw test... fixes error with IDirectDraw_CreateSurface where the test is not checking the return from the call. CreateSurface is returning a null.
Another convention is to *not* use the [PATCH] tag on wine-patches, as all mails on wine-patches are patches. Use "--keep-subject" with git-format-patch to prevent it. Last, but not least, your original subject was way too long. Subjects on wine-patches usually mention the component to be patched (that is usually the directory name without dlls or programs in front) followed by a short synopsis (around 40 to 60 characters, if possible). I suggest the following subject for your next try:
Subject: ddraw/tests: Catch failures of CreateSurface to prevent crash
I don't remember whether you had the crash on Windows or on Wine. You could put that info also into the subject. As it is more important what the goal of your patch is than how you achieve that, something like this might also be OK:
Subject: ddraw/tests: Don't crash on Windows with GeForce 4399, driver 42.23
(this subject using made-up numbers on purpose, please fit in your own, and only use this line if the crash *is* on native Windows)
diff --git a/dlls/ddraw/tests/dsurface.c b/dlls/ddraw/tests/dsurface.c index fc7bb88..21e6b15 100644 --- a/dlls/ddraw/tests/dsurface.c +++ b/dlls/ddraw/tests/dsurface.c @@ -1181,7 +1181,10 @@ static void AttachmentTest7(void) ddsd.dwHeight = GetSystemMetrics(SM_CYSCREEN); hr = IDirectDraw7_CreateSurface(dd7, &ddsd, &surface3, NULL); ok(hr==DD_OK,"CreateSurface returned: %x\n",hr);
- if (FAILED(hr)) {
skip("failed to create surface3\n");return;- } /* This one has a different size */ memset(&ddsd, 0, sizeof(ddsd)); ddsd.dwSize = sizeof(ddsd);
return inside a test function is bad style. You may use it for try&crash debugging, but I don't see any chance to get such a patch committed. In this case, you leak one IDirectDraw7 interface, forget to restore the cooperative level and you leak two surfaces. The "goto err:" approach is preferred if a lot of cleanup is needed (like here). You either want to make a label "err2:" before the IDirectDrawSurface7_Release(surface2) and use that label (analogous for "err3:" one line before that) or use the technique of setting released or uninitialized interface pointers to NULL and NULL-Check before calling release.
Same remarks for the next 3 hunks (not quoted).
Thing is the two surfaces have not been allocated yet.. they are off the local stack and just set to null.
@@ -2208,6 +2220,12 @@ static void SizeTest(void) desc.dwWidth = 128; /* Keep them set to check what happens */ ret = IDirectDraw_CreateSurface(lpDD, &desc, &dsurface, NULL); ok(ret == DD_OK, "Creating a primary surface without width and height info returned %08x\n", ret);
- /* This should be handled but CreateSurface is failing */
- if (FAILED(ret))
- {
skip("Can't create cubemap surface\n");return;- } if(dsurface) { ret = IDirectDrawSurface_GetSurfaceDesc(dsurface, &desc);
The skip message is wrong. It's not about cubemap at all. Also, this hunk is unneeded. If CreateSurface fails, dsurface is unmodified or set to NULL (or CreateSurface really badly needs fixing!). As dsurface is NULL on the call to CreateSurface, it is null if CreateSurface failed. So the condition of the if statement following the CreateSurface call is false, and no crash can happen on that NULL pointer.
Yes I just caught this message thanks .... I will fix that... The problem here which makes no sense to me is if I don't have the Failed statement before the dsurface if when it gets the surface description it throws an exception because one of the parms is null. That doesnt make sense thought because the contents of desc has been set earlier. So unless the CreateSurface call nulls out the description parm before returning. Any ideas?
@@ -2492,7 +2510,7 @@ static void PaletteTest(void) ok(hr==DD_OK, "CreateSurface returned: %x\n",hr); if (FAILED(hr)) { skip("failed to create surface\n");
- goto err;
return; }
hr = IDirectDrawSurface_SetPalette(lpSurf, palette);
Why do you need this hunk? You talked about usage of uninitialized variables at the target of the goto, but that's not the case. The code at err looks like this (formatting adjusted for clearness):
| err: | if (lpSurf) IDirectDrawSurface_Release(lpSurf); | if (palette) IDirectDrawPalette_Release(palette);
Again when I leave the goto err in and it does the release it throws an exception on the lpsurf line.
So the only two variables used here are lpSurf and palette. These two variables are initialized at the point of definition.
| static void PaletteTest(void) | { | HRESULT hr; | LPDIRECTDRAWSURFACE lpSurf = NULL; | DDSURFACEDESC ddsd; | IDirectDrawPalette *palette = NULL;
As they are initialized to NULL, the condition of the if statement is false if these variables were not yet initialized. So no crash possible here. Your return statement leaks palette.
Yes I saw that.. which is why it did not make much sense... if you want I can recreate and show you the exception which occurs when I don't have the return or the dsurface error I mentioned above. Its not that I don't know C (I have been writing in C for over 20 years so its not that.. (though I do admit at being rusty due to not doing anything but architecture and design)) . Any ideas?
BTW Thank you for your comments... I do appreciate them =) Chris