Re: [PATCH 1/4] ddraw: Don't write out if a ddraw1 surface of size 0 is requested.

[Henri Verbeet]

On 6 January 2017 at 08:07, Alex Henrie  wrote:
> Fixes https://bugs.winehq.org/show_bug.cgi?id=42159

This isn't a regression, right?
> @@ -2975,10 +2975,7 @@ static HRESULT WINAPI ddraw1_CreateSurface(IDirectDraw *iface,
>      hr = ddraw_surface_create(ddraw, &surface_desc2, &impl, outer_unknown, 1);
>      wined3d_mutex_unlock();
>      if (FAILED(hr))
> - {
> -
>    *surface = NULL;
>    return hr;
> - }

This does a bit more than the commit message says it does, this
doesn't write to *surface on any error from ddraw_surface_create().
That may very well be correct, but that's not what the test tests. The
test only tests that CreateSurface() doesn't crash on error with an
invalid "surface" pointer, but that could also be because of e.g. an
exception handler. If you want to prove that it doesn't (try to) write
to the "surface" pointer, you should pass a valid pointer to
CreateSurface() and test that it isn't modified after the call.

[PATCH 1/4] ddraw: Don't write out if a ddraw1 surface of size 0 is requested.

[Alex Henrie]

2017-01-06 4:57 GMT-07:00 Henri Verbeet :
> On 6 January 2017 at 08:07, Alex Henrie  wrote:
> > Fixes https://bugs.winehq.org/show_bug.cgi?id=42159
>
> This isn't a regression, right?
> > @@ -2975,10 +2975,7 @@ static HRESULT WINAPI ddraw1_CreateSurface(IDirectDraw *iface,
> >      hr = ddraw_surface_create(ddraw, &surface_desc2, &impl, outer_unknown, 1);
> >      wined3d_mutex_unlock();
> >      if (FAILED(hr))
> > - {
> > -
> >    *surface = NULL;
> >    return hr;
> > - }
>
> This does a bit more than the commit message says it does, this
> doesn't write to *surface on any error from ddraw_surface_create().
> That may very well be correct, but that's not what the test tests. The
> test only tests that CreateSurface() doesn't crash on error with an
> invalid "surface" pointer, but that could also be because of e.g. an
> exception handler. If you want to prove that it doesn't (try to) write
> to the "surface" pointer, you should pass a valid pointer to
> CreateSurface() and test that it isn't modified after the call.

Thanks for the feedback. No, this isn't a regression. Should I wait
until after the code freeze to submit a revised patchset?

-Alex

[PATCH 1/4] ddraw: Don't write out if a ddraw1 surface of size 0 is requested.

[Henri Verbeet]

On 6 January 2017 at 19:31, Alex Henrie  wrote:
> Thanks for the feedback. No, this isn't a regression. Should I wait
> until after the code freeze to submit a revised patchset?

Submitting it is fine, and I'll review it, but I won't sign off on it
until after the release.