On Wed, 9 Jan 2013 16:10:18 +0100 Christian Costa titan.costa@gmail.com wrote:
2013/1/9 Johannes Kroll jkroll@lavabit.com
On Wed, 09 Jan 2013 10:19:54 +0100 Christian Costa titan.costa@gmail.com wrote:
Le 09/01/2013 04:26, Johannes Kroll a écrit :
Hi,
On Wed, 09 Jan 2013 03:55:25 +0100 Christian Costa titan.costa@gmail.com wrote:
Hi,
Please be consistent when using space. 1 space before and after operators (<, ==, =, ...).
spaces inserted. HTH.
J.
That's better. You forgot some occurences in the for loop though.
Is it OK if I leave the rest of the formatting to you? Any style is fine with me, whatever makes you happy and hopefully gets the patch included.
Formatting only patch are not allowed. Clean patch and consistency in code
style help your patch get in. Alexandre Julliard is the only judge not me. If he is ok with your patch that's fine. Anyway having the for loop consistent with rest should not be that hard.
for(i= 0; i<lpMidiOutHdr->dwBufferLength; i++)
The original patch was consistent with my coding style. To make assignments stick out visually, I use no space to the left of the assignment operator, one space to the right. On comparison operators, I use space either on both sides, or none. You will see this in all code I write.
I attached another version of the patch. I added 3 more spaces and one line break in a long comment line. You or Alexandre can choose the version they like best. If you still see style issues, please consider changing the style to your liking. If that's not possible, the patch will have to be rejected.
If anybody sees non-style-related issues with the patch, please let me know.
Please keep any replies on the list(s).
Have a nice day. Johannes
2013/1/9 Johannes Kroll jkroll@lavabit.com
On Wed, 9 Jan 2013 16:10:18 +0100 Christian Costa titan.costa@gmail.com wrote:
2013/1/9 Johannes Kroll jkroll@lavabit.com
On Wed, 09 Jan 2013 10:19:54 +0100 Christian Costa titan.costa@gmail.com wrote:
Le 09/01/2013 04:26, Johannes Kroll a écrit :
Hi,
On Wed, 09 Jan 2013 03:55:25 +0100 Christian Costa titan.costa@gmail.com wrote:
Hi,
Please be consistent when using space. 1 space before and after operators (<, ==, =, ...).
spaces inserted. HTH.
J.
That's better. You forgot some occurences in the for loop though.
Is it OK if I leave the rest of the formatting to you? Any style is fine with me, whatever makes you happy and hopefully gets the patch included.
Formatting only patch are not allowed. Clean patch and consistency in
code
style help your patch get in. Alexandre Julliard is the only judge not me. If he is ok with your patch that's fine. Anyway having the for loop consistent with rest should not be that hard.
for(i= 0; i<lpMidiOutHdr->dwBufferLength; i++)
The original patch was consistent with my coding style. To make assignments stick out visually, I use no space to the left of the assignment operator, one space to the right. On comparison operators, I use space either on both sides, or none. You will see this in all code I write.
I attached another version of the patch. I added 3 more spaces and one line break in a long comment line. You or Alexandre can choose the version they like best. If you still see style issues, please consider changing the style to your liking. If that's not possible, the patch will have to be rejected.
I would say consistency would be 2 spaces or none. Using only 1 space for assignment does not seem very common in wine AFAIK. In general it's better to follow the file style or the function to avoid mixing styles except if you work heavyly on these files.
If anybody sees non-style-related issues with the patch, please let me know.
dwBytesRecorded is supposed to tell how many bytes are valid. What's wrong with it ?
Please keep any replies on the list(s).
Good mail client which sucks. wine-patches is only for patch submission.
Have a nice day. Johannes
On Wed, 9 Jan 2013 19:04:03 +0100 Christian Costa titan.costa@gmail.com wrote:
I would say consistency would be 2 spaces or none.
You are, of course, entitled to your opinion.
Using only 1 space for assignment does not seem very common in wine AFAIK. In general it's better to follow the file style or the function to avoid mixing styles except if you work heavyly on these files.
If anybody sees non-style-related issues with the patch, please let me know.
dwBytesRecorded is supposed to tell how many bytes are valid. What's wrong with it ?
Is it? It was always zero in midiOutLongMsg when I checked. The program works on Windows though. This means that either dwBytesRecorded is not supposed to be valid in that place, possibly only when data is read from some MIDI device. This would seem to be consistent with the "-Recorded" in the variable name.
Or it is supposed to be valid, which would mean that there is a bug somewhere else in Wine.
SysEx messages where the real message length equals dwBufferSize do get through properly even without the patch, which means that the code in MMDRV_Message and onwards does not care about dwBytesRecorded. It uses dwBufferSize to check how many bytes it should send.
Please keep any replies on the list(s).
Good mail client which sucks.
This sentence makes no sense.
wine-patches is only for patch submission.
Use -devel then. Just don't send me copies.
Le 09/01/2013 19:32, Johannes Kroll a écrit :
On Wed, 9 Jan 2013 19:04:03 +0100 Christian Costa titan.costa@gmail.com wrote:
I would say consistency would be 2 spaces or none.
You are, of course, entitled to your opinion.
If my opinion does not matter as well as code style practices we can found in wine that would be better to stick to file code style. Off course you're free to submit what you want.
Using only 1 space for assignment does not seem very common in wine AFAIK. In general it's better to follow the file style or the function to avoid mixing styles except if you work heavyly on these files.
If anybody sees non-style-related issues with the patch, please let me know.
dwBytesRecorded is supposed to tell how many bytes are valid. What's wrong with it ?
Is it? It was always zero in midiOutLongMsg when I checked. The program works on Windows though. This means that either dwBytesRecorded is not supposed to be valid in that place, possibly only when data is read from some MIDI device. This would seem to be consistent with the "-Recorded" in the variable name.
Or it is supposed to be valid, which would mean that there is a bug somewhere else in Wine.
SysEx messages where the real message length equals dwBufferSize do get through properly even without the patch, which means that the code in MMDRV_Message and onwards does not care about dwBytesRecorded. It uses dwBufferSize to check how many bytes it should send.
This applies only for recording which is not the case here. After a better look the changes seem correct. The condition i < lpMidioutHdr->dwBufferLength-1 is not necessary tough. And a single call to MMDRV_Message would be better.
Please keep any replies on the list(s).
Good mail client which sucks.
This sentence makes no sense.
Yours too.
wine-patches is only for patch submission.
Use -devel then. Just don't send me copies.
...
On Thu, 10 Jan 2013 00:38:06 +0100 Christian Costa titan.costa@gmail.com wrote:
After a better look the changes seem correct. The condition i < lpMidioutHdr->dwBufferLength-1 is not necessary tough.
Without the buffer length check, there would be a buffer overflow when an application tries to send a SysEx without an F7 byte.
On Thu, 10 Jan 2013 01:05:28 +0100 Johannes Kroll jkroll@lavabit.com wrote:
On Thu, 10 Jan 2013 00:38:06 +0100 Christian Costa titan.costa@gmail.com wrote:
After a better look the changes seem correct. The condition i < lpMidioutHdr->dwBufferLength-1 is not necessary tough.
Without the buffer length check, there would be a buffer overflow when an application tries to send a SysEx without an F7 byte.
I meant a read past the end of the buffer of course, not an overflow.
2013/1/10 Johannes Kroll jkroll@lavabit.com
On Thu, 10 Jan 2013 01:05:28 +0100 Johannes Kroll jkroll@lavabit.com wrote:
On Thu, 10 Jan 2013 00:38:06 +0100 Christian Costa titan.costa@gmail.com wrote:
After a better look the changes seem correct. The condition i < lpMidioutHdr->dwBufferLength-1 is not necessary
tough.
Without the buffer length check, there would be a buffer overflow when an application tries to send a SysEx without an F7 byte.
I meant a read past the end of the buffer of course, not an overflow.
In your code you stop checking F7 just before the last byte (until position lpMidiOutHdr->dwBufferLength-2); You can check the last byte as well. This is also valid and simplify the condition. The missing F7 byte case is handled by the for loop.
Regarding the single MMDRV_Message call, I was thinking about something like below. This also enable printing a WARN when F7 is missing which can be useful for debug.
DWORD oldBufferLength = lpMidiOutHdr->dwBufferLength; DWORD ret;
for(i = 0; i < lpMidiOutHdr->dwBufferLength; i++)
/* SysEx messages are terminated by a 0xF7 byte. If the buffer contains additional bytes, send only the bytes up to the termination byte. */ if((unsigned char)lpMidiOutHdr->lpData[i] == 0xF7) { lpMidiOutHdr->dwBufferLength = i+1;
break;
}
if (i == lpMidiOutHdr->dwBufferLength)
WARN("SysEx termination byte 0xF7 missing\n")
ret = MMDRV_Message(wmld, MODM_LONGDATA, (DWORD_PTR)lpMidiOutHdr, uSize);
/* restore the midi header to its original state. */ lpMidiOutHdr->dwBufferLength = oldBufferLength;
return ret;
On Thu, 10 Jan 2013 12:04:02 +0100 Christian Costa titan.costa@gmail.com wrote:
In your code you stop checking F7 just before the last byte (until position lpMidiOutHdr->dwBufferLength-2); You can check the last byte as well. This is also valid and simplify the condition. The missing F7 byte case is handled by the for loop.
Ah, you mean that check. Yes, it's not strictly necessary, it just decides whether it has to save & restore dwBufferSize or not.
Regarding the single MMDRV_Message call, I was thinking about something like below. This also enable printing a WARN when F7 is missing which can be useful for debug.
DWORD oldBufferLength = lpMidiOutHdr->dwBufferLength; DWORD ret; for(i = 0; i < lpMidiOutHdr->dwBufferLength; i++) /* SysEx messages are terminated by a 0xF7 byte. If the buffercontains additional bytes, send only the bytes up to the termination byte. */ if((unsigned char)lpMidiOutHdr->lpData[i] == 0xF7) { lpMidiOutHdr->dwBufferLength = i+1;
break; } if (i == lpMidiOutHdr->dwBufferLength) WARN("SysEx termination byte 0xF7 missing\n") ret = MMDRV_Message(wmld, MODM_LONGDATA, (DWORD_PTR)lpMidiOutHdr,uSize);
/* restore the midi header to its original state. */ lpMidiOutHdr->dwBufferLength = oldBufferLength; return ret;
I see nothing wrong with that apart from the broken formatting (please don't use HTML mail). Did you test it?
I don't care which version is used, I would just be very happy if *something* is included which makes my MIDI device work with Wine.
How does this generally work here, will there be feedback from a maintainer on whether the patch will be included, or is it usually just silently included?
Alexandre Julliard (or whoever decides this): Could you comment please?
And Christian, please, configure your mail client. The list mails I get from you are directed to me, not the list, and they are missing the List-Id field.
2013/1/10 Johannes Kroll jkroll@lavabit.com
On Thu, 10 Jan 2013 12:04:02 +0100 Christian Costa titan.costa@gmail.com wrote:
In your code you stop checking F7 just before the last byte (until position lpMidiOutHdr->dwBufferLength-2); You can check the last byte as well. This is also valid and simplify the condition. The missing F7 byte case is handled by the for loop.
Ah, you mean that check. Yes, it's not strictly necessary, it just decides whether it has to save & restore dwBufferSize or not.
Regarding the single MMDRV_Message call, I was thinking about something like below. This also enable printing a WARN when F7 is missing which can be useful
for
debug.
DWORD oldBufferLength = lpMidiOutHdr->dwBufferLength; DWORD ret; for(i = 0; i < lpMidiOutHdr->dwBufferLength; i++) /* SysEx messages are terminated by a 0xF7 byte. If the buffercontains additional bytes, send only the bytes up to the termination byte. */ if((unsigned char)lpMidiOutHdr->lpData[i] == 0xF7) { lpMidiOutHdr->dwBufferLength = i+1;
break; } if (i == lpMidiOutHdr->dwBufferLength) WARN("SysEx termination byte 0xF7 missing\n") ret = MMDRV_Message(wmld, MODM_LONGDATA, (DWORD_PTR)lpMidiOutHdr,uSize);
/* restore the midi header to its original state. */ lpMidiOutHdr->dwBufferLength = oldBufferLength; return ret;I see nothing wrong with that apart from the broken formatting (please don't use HTML mail). Did you test it?
It was not intended to be used as is. It's just to show what I had in mind.
I don't care which version is used, I would just be very happy if *something* is included which makes my MIDI device work with Wine.
There is no reason your patch will not get in. Generally, it's just patches need to have the quality required to be committed. Sometime it can takes several iterations.
How does this generally work here, will there be feedback from a maintainer on whether the patch will be included, or is it usually just silently included?
Alexandre Julliard (or whoever decides this): Could you comment please?
For a patch which is ok, it will be committed silently by Alexandre. You may already know the patches status page at http://source.winehq.org/patches. Usually maintainers or skilled people of a specific area look at patches and give some feedback if something seems wrong or not clear.
And Christian, please, configure your mail client. The list mails I get from you are directed to me, not the list, and they are missing the List-Id field.
When I use gmail web client, I don't have good results sometimes. Maybe
there is a configuration for that. I'm full open to any hint. Regarding emails addresses, I write to someone with wine-devel in cc but I'm ok to use only wine-devel with you.
-
Christian Costa -
Johannes Kroll