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

Support seeking also from the end of file on S3 storage #28802

Merged
merged 1 commit into from
Sep 14, 2021

Conversation

paulijar
Copy link
Contributor

The PR #20033 added support for fseek for the S3 storage backend. However, the seek mode SEEK_END was left out that time. This PR adds this support.

The motivation for this was that the getID3 library uses the seek mode SEEK_END and doesn't work properly without it. After the PR JamesHeinrich/getID3#328 got merged, the getID3 library no longer works at all on file system where fseek returns error with any used seek mode. This was the root cause for the issue owncloud/music#887.

The PR nextcloud#20033 added support
for `fseek` for  the S3 storage backend. However, the seek mode SEEK_END
was left out that time. This PR fills this gap.

Signed-off-by: Pauli Järvinen <pauli.jarvinen@gmail.com>
@szaimen szaimen added the 3. to review Waiting for reviews label Sep 12, 2021
@szaimen szaimen added this to the Nextcloud 23 milestone Sep 12, 2021
@szaimen szaimen requested review from a team, PVince81 and blizzz and removed request for a team September 12, 2021 13:10
Copy link
Contributor

@kesselb kesselb left a comment

Choose a reason for hiding this comment

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

Thanks 👍

@juliushaertl
Copy link
Member

Failure unrelated

@juliushaertl juliushaertl merged commit 8e7f63b into nextcloud:master Sep 14, 2021
@paulijar paulijar deleted the enh/s3_seek_from_end branch September 14, 2021 12:18
@kesselb
Copy link
Contributor

kesselb commented Sep 14, 2021

does it make sense to backport?

@paulijar
Copy link
Contributor Author

does it make sense to backport?

In my opinion, yes. This fixes an actual problem, and the patch should be applicable all the way starting from NC17.

@paulijar
Copy link
Contributor Author

So who should decide if this gets backported?

@kesselb
Copy link
Contributor

kesselb commented Sep 20, 2021

/backport to stable22

@kesselb
Copy link
Contributor

kesselb commented Sep 20, 2021

/backport to stable21

@kesselb
Copy link
Contributor

kesselb commented Sep 20, 2021

/backport to stable20

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants