Skip to contentSkip to conversation

lzo: fix possible typo in decompresor

PATCH In review Harvey Harrison 3 participants 5 messages Apr 10, 2008

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 lzo: fix possible typo in decompresor

Shift of a le value seems strange, probably meant to shift the cpu-order variable as in the prvious section of the switch statement.

---
 lib/lzo/lzo1x_decompress.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
lib/lzo/lzo1x_decompress.c +1 -1
index 9dc7056..77f0f9b 100644
@@ -158,7 +158,7 @@ match:
 					t += 7 + *ip++;
 				}
 				m_pos -= le16_to_cpu(get_unaligned(
-					(const unsigned short *)ip) >> 2);
+					(const unsigned short *)ip)) >> 2;
 				ip += 2;
 				if (m_pos == op)
 					goto eof_found;
Signature · 1 line
1.5.5.144.g3e42
#1 lore raw
Quoted 3 lines · Harvey Harrison

Shift of a le value seems strange, probably meant to shift the cpu-order variable as in the prvious section of the switch statement.

Hmm. This patch looks ObviouslyCorrect(tm), but it worries me that apparently the old broken code has been around since last July, and afaik it can never have worked on big-endian machines.

So did nobody ever use it, or why hasn't this ever triggered? How did you find this? A sparse warning?

Linus

#2 lore raw
Quoted 6 lines · Linus Torvalds
Quoted 3 lines · Harvey Harrison

Shift of a le value seems strange, probably meant to shift the cpu-order variable as in the prvious section of the switch statement.

Quoted 8 lines

Hmm. This patch looks ObviouslyCorrect(tm), but it worries me that apparently the old broken code has been around since last July, and afaik it can never have worked on big-endian machines.

So did nobody ever use it, or why hasn't this ever triggered? How did you find this? A sparse warning?

Linus

When converting in tree users to my proposed unaligned byteswapping api. See the 8 patch series posted today to linux-arch.

Harvey

#3 lore raw
In reply to Linus Torvalds
Quoted 11 lines · Linus Torvalds
Quoted 3 lines · Harvey Harrison

Shift of a le value seems strange, probably meant to shift the cpu-order variable as in the prvious section of the switch statement.

Hmm. This patch looks ObviouslyCorrect(tm), but it worries me that apparently the old broken code has been around since last July, and afaik it can never have worked on big-endian machines.

So did nobody ever use it, or why hasn't this ever triggered? How did you find this? A sparse warning?

The heaviest users of the lzo code I know of are little-endian ARM devices through jffs2. When the code was merged there was a lot of discussion about the best way to handle the endian issues and unaligned accesses and whilst I seem to remember someone posting big-endian test results it could have been before some of the later changes were made.

So yes its possible its not been run on BE until now or that isn't a common code path. I've checked this against the external LZO library its based on and the patch is correct

Signature · 1 line
Richard
#4 lore raw
Quoted 19 lines · Richard Purdie
Quoted 11 lines · Linus Torvalds
Quoted 3 lines · Harvey Harrison

Shift of a le value seems strange, probably meant to shift the cpu-order variable as in the prvious section of the switch statement.

Hmm. This patch looks ObviouslyCorrect(tm), but it worries me that apparently the old broken code has been around since last July, and afaik it can never have worked on big-endian machines.

So did nobody ever use it, or why hasn't this ever triggered? How did you find this? A sparse warning?

The heaviest users of the lzo code I know of are little-endian ARM devices through jffs2. When the code was merged there was a lot of discussion about the best way to handle the endian issues and unaligned accesses and whilst I seem to remember someone posting big-endian test results it could have been before some of the later changes were made.

Quoted 6 lines

So yes its possible its not been run on BE until now or that isn't a common code path. I've checked this against the external LZO library its based on and the patch is correct

This is one of the reasons I thought about for adding the new api, the bracketing is just too easy to get wrong when you throw get/put_unaligned into the mix.

Harvey