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.

Sign up for GitHub

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

Jump to bottom

[stable9] minor smb logging, do not use deprecated method #25994

Closed
wants to merge 2 commits into from
Closed

[stable9] minor smb logging, do not use deprecated method #25994

wants to merge 2 commits into from

Conversation

butonic
Copy link
Member

@butonic butonic commented Aug 31, 2016

add log for the actual smb implementation that is used (libsmbclient vs smbclient)

already contained in master and stable9 PRs

cc @felixboehm @DeepDiver1975 @PVince81

@mention-bot
Copy link

@butonic, thanks for your PR! By analyzing the annotation information on this pull request, we identified @icewind1991, @jmaciasportela, @jvillafanez and @Xenopathic to be potential reviewers

@butonic butonic changed the title minor smb logging, do not use deprecated method [stable9] minor smb logging, do not use deprecated method Aug 31, 2016
@butonic butonic added this to the 9.0.5 milestone Aug 31, 2016
VicDeo
VicDeo reviewed Aug 31, 2016
View reviewed changes
apps/files_external/lib/smb.php
@@ -410,7 +412,7 @@ public function fopen($path, $mode) {
if (!$this->isCreatable(dirname($path))) {
break;
}
$tmpFile = \OCP\Files::tmpFile($ext);
$tmpFile = \OC::$server->getTempManager()->getTemporaryFile($ext);
Copy link
Member

Choose a reason for hiding this comment

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

@butonic interrresting. Why we are creating file with the name which is either strrpos($path, '.') or empty string in the first place?

Copy link
Member

Choose a reason for hiding this comment

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

and what happens if any directory in $path has a dot: /something/that/is/not.supposed/tohappen

Copy link
Member Author

@butonic butonic Aug 31, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

um from phpdoc * @param string $postFix Postfix appended to the temporary file name. so $ext is just appended to have the same filetype if it is given. That being said /something/that/is/not.supposed/tohappen is a valid point. I guess

  • dropping the $ext is fine.
  • add a PR for a unit test that extends directory providers with /something/that/is/not.supposed/tohappen

@VicDeo good catch!

@PVince81
Copy link
Contributor

PVince81 commented Sep 7, 2016

any update ? would move to 9.0.6

@butonic butonic mentioned this pull request Sep 19, 2016
Closed
9 tasks
@butonic
Copy link
Member Author

butonic commented Sep 19, 2016

tests are coming out green. @PVince81 @VicDeo @jvillafanez

@butonic butonic mentioned this pull request Sep 19, 2016
Closed
@butonic
Copy link
Member Author

butonic commented Sep 19, 2016

I take that back:


12:29:14 1) Test\Files\Storage\DAV::testWriteWithDotInPath
12:29:14 OCP\Files\StorageNotAvailableException: Sabre\HTTP\ClientHttpException: Conflict
12:29:14 
12:29:14 /var/lib/jenkins/workspace/owncloud-core/core/PR-25994/lib/private/files/storage/dav.php:797
12:29:14 /var/lib/jenkins/workspace/owncloud-core/core/PR-25994/lib/private/files/storage/dav.php:638
12:29:14 /var/lib/jenkins/workspace/owncloud-core/core/PR-25994/lib/private/files/storage/dav.php:177
12:29:14 /var/lib/jenkins/workspace/owncloud-core/core/PR-25994/tests/lib/files/storage/storage.php:152

investigating

@butonic butonic modified the milestones: 9.0.6, 9.0.5 Sep 19, 2016
@jvillafanez
Copy link
Member

Changes look good expect for the failing test.

@butonic butonic self-assigned this Sep 21, 2016
@PVince81 PVince81 modified the milestones: 9.0.7, 9.0.6 Nov 9, 2016
@PVince81
Copy link
Contributor

PVince81 commented Nov 9, 2016

Moving to 9.0.7.

@PVince81
Copy link
Contributor

@butonic any update ?

@PVince81 PVince81 modified the milestones: 9.0.8, 9.0.7 Nov 30, 2016
@PVince81 PVince81 modified the milestones: 9.0.9, 9.0.8 Apr 13, 2017
@PVince81
Copy link
Contributor

is this backport still worth it ?

@PVince81 PVince81 modified the milestones: 9.0.10, 9.0.9 Apr 13, 2017
@PVince81 PVince81 modified the milestones: 9.0.10, 9.0.11 May 29, 2017
@PVince81 PVince81 modified the milestones: triage, 9.0.11 Jun 29, 2017
@PVince81
Copy link
Contributor

PVince81 commented Jul 4, 2017

no feedback since sep 2016, closing

@PVince81 PVince81 closed this Jul 4, 2017
@DeepDiver1975 DeepDiver1975 deleted the stable9-minor-smb-log branch August 11, 2017 22:54
@felixboehm felixboehm removed this from the triage milestone Apr 10, 2018
@lock
Copy link

lock bot commented Jul 31, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jul 31, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@butonic butonic

Labels
2 - Developing app:files_external enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@butonic @mention-bot @PVince81 @jvillafanez @VicDeo @felixboehm