Incorrect error semantics in fluid_midi.c

[derselbst]

Sorry to bring this up again -- it's a minor nit-pick, but I am trying to rewrite the implementation of several functions in fluid_midi.c without changing the interface, and the current interface is a bit weird. I'm putting two issues into the one bug report since they're related and minor.

Both of these problems came up in r392, when plcl closed ticket #92. It's very hard to tell from the diff (http://fluidsynth.svn.sourceforge.net/viewvc/fluidsynth/trunk/fluidsynth/src/midi/fluid_midi.c?r1=392&r2=391&pathrev=392) because a lot changed in one commit.

The first issue is in fluid_midi_file_getc, in response to my patch on ticket #92. On line 104, the line "return FLUID_FAILED" was added. However, in my patch I had "return -1". Now FLUID_FAILED just happens to be defined as -1 (so it still works), but this is the wrong constant. Most functions return FLUID_FAILED and the calls check "if (result != FLUID_OK)", but this is not the case with fluid_midi_file_getc (which is emulating fgetc).

All of the calls to fluid_midi_file_getc check for the EOF condition with "if (result < 0)". Therefore, it is correct to return -1 to match these checks. If FLUID_FAILED were changed to a different constant, this code would break. It should be changed to "return -1" (or "return EOF" if you want to use a constant with the same semantics as fgetc).

The second issue is in fluid_midi_file_read. I am not sure what the intention behind this was, but note that on line 128, the check "if (num == len)" was added, preventing mf->trackpos from being incremented if fewer than len bytes were read. Now again, this doesn't matter too much, since it will subsequently return FLUID_FAILED and this will pretty much terminate the loading of the file. But in principle, a bug has been introduced here: the FILE*'s internal read pointer has been advanced by some number of bytes (less than len but possibly greater than 0), but the trackpos has not been incremented, so the state of mf is inconsistent. I would recommend that this be reverted such that mf->trackpos is always incremented regardless of the success or failure of the function.

I have attached a small patch for each of these issues.

Reported by: mattgiuca

Original Ticket: fluidsynth/tickets/94

[derselbst]

• attachment _fluid_midi_file_getc.patch_ added

Patch for fluid_midi_file_getc

Original comment by: mattgiuca

[derselbst]

• attachment _fluid_midi_file_read.patch_ added

Patch for fluid_midi_file_read

Original comment by: mattgiuca

[derselbst]

• cc jerseychewi added

Original comment by: jerseychewi

[derselbst]

All of the calls to fluid_midi_file_getc check for the EOF condition with "if (result < 0)". Therefore, it is correct to return -1 to match these checks.

You are probably right. However this change exists now for 7 years. And as it was documented properly, I'm not willing to change it back again.

I agree with the second issue and will apply the change.

[derselbst]

Though I agree with the second issue it now contains a note:

Read bytes, even if there aren't enough, but only increment trackpos if successful (emulates old behaviour of fluid_midi_file_read)

For now I'm not willing to change this. Postponed until next major release.