Try #2

[Michael Karcher]

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).

> @@ -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.


> @@ -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);

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.

Regards,
  Michael Karcher