dsound: Don't bother shrinking the secondary buffer list.

Michael,
> +#include <assert.h>

your patch is already committed, yet I believe that the least that dsound needs is new asserts.

Asserts in dsound have been a PITA in Wine for the last decade.

It's ok for the kernel to crash in an assert. IMHO it is not ok for an optional thing like sound.

The assert's backtrace will not help anybody, because the logical error will have happened much earlier.

Instead, a plain ERR would have been just as telling. Let's quote
http://www.winehq.org/docs/winedev-guide/debugging#DBG-CLASSES
"ERR: Messages in this class indicate serious errors in Wine,
          such as as conditions that should never happen by design."

I recommend:
-    assert(device->buffers[0] == pDSB);
+   if (pdevice->buffers[0] != pDSB)
+       ERR("broken device buffer %p\n", device->buffers[0]);
      HeapFree(GetProcessHeap(), 0, device->buffers);
      device->buffers = NULL;

The ERR is IMHO revealing enough in a user's log.

If a sound component detects a bogus state,
I'd very much prefer that it disables sound -- without crashing.

Regards,
 Jörg Höhle

[Michael Stefaniuc]

Hello Jörg,

as always it depends.

On 09/27/2012 09:37 AM, Joerg-Cyril.Hoehle(a)t-systems.com wrote:
> > +#include <assert.h>
>
> your patch is already committed, yet I believe that the least that dsound needs is new asserts.
>
> Asserts in dsound have been a PITA in Wine for the last decade.
>
> It's ok for the kernel to crash in an assert. IMHO it is not ok for an optional thing like sound.
>
> The assert's backtrace will not help anybody, because the logical error will have happened much earlier.
> I disagree. It helps GyB to open regression bug reports and it helps me
> to fix them.
> Instead, a plain ERR would have been just as telling. Let's quote
> http://www.winehq.org/docs/winedev-guide/debugging#DBG-CLASSES
> "ERR: Messages in this class indicate serious errors in Wine,
>           such as as conditions that should never happen by design."
>
> I recommend:
> -    assert(device->buffers[0] == pDSB);
> +   if (pdevice->buffers[0] != pDSB)
> +       ERR("broken device buffer %p\n", device->buffers[0]);
>       HeapFree(GetProcessHeap(), 0, device->buffers);
>       device->buffers = NULL;
>
> The ERR is IMHO revealing enough in a user's log.
> That's too late if it hits the user.

I had reviewed the code: That code is an implementation detail
completely outside the control of the applications. Barring a memory
corruption there is no way for it to fail. The assert is there for two
reasons:
- To crash during "make test" when the next developer touches that code.
- As a comment less likely to be removed by Alexandre ;)
> If a sound component detects a bogus state,
> I'd very much prefer that it disables sound -- without crashing.
> In general "it depends" of course.

In this particular case the ERR would be useless noise as the assert is
only for the last sound buffer that is removed; usually at program exit.
Until then the code does what you prefer: silently drops sound buffers
without crashing thus deferring the crash to the program exit. A crash
on program exit does get reported, more so than an ERR on the command line.

bye
	michael