[alsa-devel] [PATCH 2/3] ALSA: tascam: improve loop condition to filling MIDI bytes in transaction

[Takashi Iwai]

On Sun, 18 Oct 2015 12:51:33 +0200,
Takashi Sakamoto wrote:
> 
> On Oct 18 2015 19:10, Takashi Iwai wrote:
> > On Sun, 18 Oct 2015 12:02:17 +0200,
> > Takashi Sakamoto wrote:
> >>
> >> It includes meaningless condition. This commit improve it.
> >>
> >> Signed-off-by: Takashi Sakamoto <o-takashi at sakamocchi.jp>
> >> ---
> >>  sound/firewire/tascam/tascam-transaction.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/sound/firewire/tascam/tascam-transaction.c b/sound/firewire/tascam/tascam-transaction.c
> >> index 7fcee81..4ffaa8a 100644
> >> --- a/sound/firewire/tascam/tascam-transaction.c
> >> +++ b/sound/firewire/tascam/tascam-transaction.c
> >> @@ -74,7 +74,7 @@ static int fill_message(struct snd_rawmidi_substream *substream, u8 *buf)
> >>  	/* On exclusive message. */
> >>  	if (tscm->on_sysex[port]) {
> >>  		/* Seek the end of exclusives. */
> >> -		for (i = 1; i < 4 || i < len; ++i) {
> >> +		for (i = 1; i < len + 1; ++i) {
> > 
> > This doesn't look like an improvement...
> > The fix should be rather like:
> > 		for (i = 1; i < 4 && i < len; ++i) {
> 
> The 'len' variable is assigned to the return value of
> snd_rawmidi_transmit_peek(). The third argument to call this function is
> 3, thus the value of this variable equals to or is less than 3.
> 
> The aim of this loop is to find EOX (end of exclusive) byte in retrieved
> bytes, then break the loop.The index goes from 1 to the end of retrieved
> bytes. Thus, 'i < 4 || i < len' is not so proper, here.

Then the patch description is incorrect.  You actually fix something
that was a buggy behavior.

Overall, the code is confusing because the real data buffer begins at
offset 1 and len doesn't mean the size of the values written to buf[].
Maybe it's better to be like:
	char *msg = buf + 1;

and use msg[x] instead of buf[1+x].  Then len really corresponds to
the size of msg[].


Takashi