479929 - Return value of esd_open_sound not checked correctly

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Matthew Gregan [:kinetik]]

esd_open_sound returns a file descriptor, we test for (!fd) when we should test for (fd < 0).

Note: this is a spin off of my patch from bug 469635.

Comment 1

[Matthew Gregan [:kinetik]]

Attachment: patch v1

Also removes confusing/useless comments.

Comment 2

[Matthew Gregan [:kinetik]]

Pushed 5ac18226837e.

Comment 4

[Wolfgang Rosenauer [:wolfiR]]

Comment on attachment 363833 [details] [diff] [review]
patch v1

>     if (!elib) {
>-        /* we don't need to do esd_open_sound if we are only going to play files
>-           but we will if we want to do things like streams, etc */
>-        EsdOpenSoundType EsdOpenSound;
>-
>         elib = PR_LoadLibrary("libesd.so.0");
>         if (elib) {
>-            EsdOpenSound = (EsdOpenSoundType) PR_FindFunctionSymbol(elib, "esd_open_sound");
>+            EsdOpenSoundType EsdOpenSound =
>+                (EsdOpenSoundType) PR_FindFunctionSymbol(elib, "esd_open_sound");
>             if (!EsdOpenSound) {
>                 PR_UnloadLibrary(elib);
>                 elib = nsnull;
>             } else {
>                 esdref = (*EsdOpenSound)("localhost");
>-                if (!esdref) {
>+                if (esdref < 0) {

I consider this patch/fix broken. It makes sense at first glance but it doesn't fit
reality AFAICS.

1. The error return code of esd_open_sound() is defined as -1 in any case (no need to fiddle with that)
2. The comment which was removed /* we don't need to do esd_open_sound if we are only going to play files */
   seems to be perfectly true. The usecase of notification in TB, SM and FF is playing WAV files.
   This definitely works w/o esd running and therefore the !esdref makes more sense than esdref < 0
   The new implementation unloads libesd just because no esd is running which is done under the
   assumption that libesd cannot output anything w/o esd running what is not true.

Comment 5

[Wolfgang Rosenauer [:wolfiR]]

reopening (or do we need a new bug?)

Comment 6

[Wolfgang Rosenauer [:wolfiR]]

Attachment: patch reverting the change

Comment 7

[Wolfgang Rosenauer [:wolfiR]]

Created new bug 579877 to allow proper tracking.