todo_wine, broken() and a SW anti-pattern

[Joerg-Cyril.Hoehle at t-systems.com]

Hi,

The question about how to use todo_wine and broken() comes up from
time to time. Here's new light on it, as I'm suggesting in mmdevapi/tests/render.c:

hr = IAudioClient_GetService(ac, &IID_IAudioStreamVolume, (void**)&out);
ok(hr == AUDCLNT_E_NOT_INITIALIZED, "... call returns %08x\n", hr);
todo_wine ok(broken(out != NULL), "GetService %08x &out pointer %p\n", hr, out);

1. broken() documents that I consider native's observable behaviour
   broken because I judge it unsafe not to zero the out pointer in case
   of failure.  Furthermore, MSDN documents that it should be zeroed.
http://msdn.microsoft.com/en-us/library/windows/desktop/dd370873%28v=vs.85%29.aspx

2. Not writing ok(broken(...) || <other condition>)
   allows to detect should MS ever change behaviour.  Currently,
   neither Vista, w2k8 nor w7 implement the documented behavior.

3. By using todo_wine, I acknowledge that Wine should continue to
   disagree with native's behavior and safely reset the out pointer.
   I mentioned "wine_dont" in
http://www.winehq.org/pipermail/wine-devel/2011-August/091764.html

   That's not "100% bug compatible", but there's a small chance that
   it helps apps *not* crash in Wine when Wine's GetService returns
   E_NOTIMPL or some such that might never occur in native, hence the
   app is not prepared for it.

More generally, a function with both a return code and an out pointer
is often misused.  Typically, the specification says:
  "The out pointer is only set when the return code indicates success".
But actually, many program(mer)s use:
  "The pointer is non-NULL if and only if the return code indicates success."

Here's an example of broken code that follows:
 /* When FAILED(hr), it is *not* the case that out == NULL.
  * Hence the anti-pattern:
  */
     hr = GetService(..., &out);
     if (SUCCEEDED(hr)) {
         ...
         if (!...) goto exit;
         ...
     }
   exit:
     if (out) { Release(out); } /* bogus */

I once found such a case in Wine but I forgot where...
(not just in mmdevapi/tests/render.c which after all is "only" test code)
Michael, a challenge for your static analysis tools?

Bogus too: set out = NULL prior to calling the function, because
you don't know whether the function may write something initially,
only to bail out later with an error code such as OUTOFMEMORY.

Regards,
	Jörg Höhle