Redirect unlink to rmdir #27101

Merged
merged 1 commit into from Feb 8, 2017

Conversation

Projects
None yet
4 participants
@PVince81
Member

PVince81 commented Feb 7, 2017

Description

Many API callers will call unlink even for directories and it can mess
up with some wrappers like the encryption wrapper

Related Issue

None raised. Related issue but this doesn't solve it: #8630

Motivation and Context

How Has This Been Tested?

  1. Disable trashbin
  2. Setup SMB ext storage
  3. Create a folder
  4. Setup a breakpoint in SMB storage class' fopen method
  5. Delete the folder

Before the fix: breakpoint stops in fopen. On some setups the SMB lib will complain about "InvalidTypeException"
After the fix: deletion works, fopen not called.

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@jvillafanez @DeepDiver1975

I'm not sure about a backport... feels a bit risky.

Redirect unlink to rmdir
Many API callers will call unlink even for directories and it can mess
up with some wrappers like the encryption wrapper

@PVince81 PVince81 added this to the 10.0 milestone Feb 7, 2017

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Feb 7, 2017

@PVince81, thanks for your PR! By analyzing the history of the files in this pull request, we identified @DeepDiver1975, @tanghus and @butonic to be potential reviewers.

@PVince81, thanks for your PR! By analyzing the history of the files in this pull request, we identified @DeepDiver1975, @tanghus and @butonic to be potential reviewers.

@mmattel

This comment has been minimized.

Show comment
Hide comment
@mmattel

mmattel Feb 7, 2017

Contributor

I have not tested it, but it looks good 👍

Contributor

mmattel commented Feb 7, 2017

I have not tested it, but it looks good 👍

@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
Member

DeepDiver1975 commented Feb 8, 2017

👍

@DeepDiver1975 DeepDiver1975 merged commit 41ef8e3 into master Feb 8, 2017

4 checks passed

Scrutinizer 4 new issues, 1 updated code elements
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details

@DeepDiver1975 DeepDiver1975 deleted the convert-unlink-rmdir branch Feb 8, 2017

@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
Member

DeepDiver1975 commented Feb 8, 2017

@PVince81 backport?

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Feb 8, 2017

Member

Backport feels a bit risky. I need to think about what could go wrong in past releases with this...

Member

PVince81 commented Feb 8, 2017

Backport feels a bit risky. I need to think about what could go wrong in past releases with this...

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Feb 10, 2017

Member

Oh well, let's try it... Integration tests will tell.

So far I can't think of specific cases.

Member

PVince81 commented Feb 10, 2017

Oh well, let's try it... Integration tests will tell.

So far I can't think of specific cases.

PVince81 added a commit that referenced this pull request Feb 10, 2017

Redirect unlink to rmdir (#27101)
Many API callers will call unlink even for directories and it can mess
up with some wrappers like the encryption wrapper
@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Feb 10, 2017

Member

stable9.1: #27126
stable9: #27127
stable8.2: #27128

Member

PVince81 commented Feb 10, 2017

stable9.1: #27126
stable9: #27127
stable8.2: #27128

PVince81 added a commit that referenced this pull request Feb 10, 2017

Redirect unlink to rmdir (#27101)
Many API callers will call unlink even for directories and it can mess
up with some wrappers like the encryption wrapper

PVince81 added a commit that referenced this pull request Feb 10, 2017

Redirect unlink to rmdir (#27101)
Many API callers will call unlink even for directories and it can mess
up with some wrappers like the encryption wrapper

@MorrisJobke MorrisJobke referenced this pull request in nextcloud/server Mar 17, 2017

Merged

Redirect unlink to rmdir #3892

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment