Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix oc stream seek #11204

Merged
merged 4 commits into from Sep 22, 2014
Merged

Fix oc stream seek #11204

merged 4 commits into from Sep 22, 2014

Conversation

butonic
Copy link
Member

@butonic butonic commented Sep 21, 2014

I was debugging a strange bug for fun in my vacation: owncloud/music#212

In short fseek end ftell are broken when using the OC stream wrapper. The stream_seek implementation needs to return true or false, depending on the result of fseek, see http://de1.php.net/manual/de/streamwrapper.stream-seek.php#115111

test in ef6db5e
fix in 01e61bf

cc @icewind1991 @MorrisJobke

@karlitschek I recommend backporting unless we want to hunt obscure problems in the enterprise edition

@MorrisJobke
Copy link
Contributor

@butonic Thanks. I will test this tomorrow.

@ghost
Copy link

ghost commented Sep 21, 2014

🚀 Test Passed. 🚀
Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser/7491/

@@ -48,7 +48,10 @@ public function stream_open($path, $mode, $options, &$opened_path) {
}

public function stream_seek($offset, $whence = SEEK_SET) {
fseek($this->fileSource, $offset, $whence);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return fseek(...) === 0;

@MorrisJobke
Copy link
Contributor

Works 👍

@MorrisJobke
Copy link
Contributor

@fossxplorer @lseabra Check this out. This fixed the memory allocation issue in the music app for the files I got from you.

Sidenote: I collected problematic files and checked them with this PR and all problems with the memory allocation are gone.

@MorrisJobke
Copy link
Contributor

@karlitschek I would also vote for the backport.

@scrutinizer-notifier
Copy link

A new inspection was created.

@butonic
Copy link
Member Author

butonic commented Sep 22, 2014

@icewind1991 I updated all wrappers to use the same === comparison

@icewind1991
Copy link
Contributor

👍

@MorrisJobke
Copy link
Contributor

Still 👍

@karlitschek
Copy link
Contributor

👍 very coool. Please backport

@ghost
Copy link

ghost commented Sep 22, 2014

🚀 Test PASSed. 🚀
Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser/7499/

MorrisJobke added a commit that referenced this pull request Sep 22, 2014
@MorrisJobke MorrisJobke merged commit 9e8d290 into master Sep 22, 2014
@MorrisJobke MorrisJobke deleted the fix_oc_stream_seek branch September 22, 2014 14:25
@MorrisJobke
Copy link
Contributor

stable7 dfc6bfe 2bdf5a8 e556698 44c7562

@lseabra
Copy link

lseabra commented Sep 22, 2014

The changes did allow me to scan my music folder, thanks guys!

@lock lock bot locked as resolved and limited conversation to collaborators Aug 16, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants