new SMB storage backend #4770

icewind1991 · 2013-09-08T13:30:59Z

New implementation of the SMB storage backend using a new wrapper for smbclient (https://github.com/icewind1991/SMB).
This improves the test-ability and maintainability of the SMB backend and hopefully helps with the various issues the current SMB backend has.

Tested it locally against an smb server running on arch, requires testing against a windows smb server.

cc @DeepDiver1975 @karlitschek

ghost · 2013-09-08T13:37:22Z

Test failed.
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/939/

ghost · 2013-09-08T15:33:36Z

Test passed.
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/941/

DeepDiver1975 · 2013-09-09T19:41:04Z

apps/files_external/lib/smb.php

+	}
+
+	public function isUpdatable($path) {
+		return true;


hmm - smb/cifs shares can be read only as well

DeepDiver1975 · 2013-09-09T21:41:23Z

@icewind1991
Tested it against a Windows 7 machine
testRoot and testMimeType are failing
testFOpen and all remaining test could not be executed because a lot of smbclient processes (one for each executed test) consumed all the cpu - there is something wrong with the process termination of smbclient

Regarding testRoot:
it failes with root being '', '/', '/test and 'test'

for further testing: grab a windows vm here: http://www.modern.ie/en-US/virtualization-tools#downloads
Feel free ti catch me on irc in case you need help with the setup of shares in windows

ghost · 2013-09-09T23:35:52Z

Test passed.
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/953/

itn3rd77 · 2013-09-17T08:54:44Z

@icewind1991 Did you keep in mind the issue #2502 when implementing the new SMB storage backend?

DeepDiver1975 · 2013-10-09T10:06:34Z

OC7

PVince81 · 2013-11-19T08:41:31Z

@icewind1991 I haven't read the code yet. Will it support streaming ? (ie not having to download a whole file when only the first MB is needed, for example for thumbnail generation or mimetype detection)

icewind1991 · 2013-11-19T12:42:15Z

No, that is a limitation of smbclient itself

PVince81 · 2013-11-20T13:37:45Z

Would be good to check whether it's possible to tell smbclient to output the file contents to stdout instead of a target file (or pipe it). If that works, it might be possible to stream whatever comes from its stdout back to ownCloud, maybe using popen() and pipes.

PVince81 · 2013-11-21T17:09:00Z

Awwwyeah, streaming/piping could work, this worked for me:

smbclient \\\\localhost\\shared -N -c "get pic.jpg -"

Seems it can work with put as well:

cat pic.jpg | smbclient \\\\localhost\\shared -N -c "put - pic.jpg"

PVince81 · 2013-11-28T11:45:14Z

apps/files_external/lib/smb.php

+	}
+
+	public function isReadable($path) {
+		return true;


This needs to either use file_exists() or fallback to the default impl that does just that.
Same for isUpdatable().
Always returning true might make callers believe that a file exists when it doesn't (scanFile())

Conflicts: apps/files_external/3rdparty/smb4php/smb.php apps/files_external/lib/smb.php

…rename not overwriting

icewind1991 · 2013-12-17T13:45:58Z

Updated streamwrapper and storage.

tested against a windows 8 VM and a samba server on Arch Linux.

scrutinizer-notifier · 2014-02-25T14:19:33Z

A new inspection was created.

ghost · 2014-02-25T15:49:09Z

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/3328/

bantu · 2014-03-06T14:18:46Z

apps/files_external/3rdparty/smb/autoload.php

+
+require_once 'errors.php';
+
+spl_autoload_register(function ($class) {


Can't this be done in a more automatic-autoload-approach, i.e. without registering your own autoloader?

bantu · 2014-03-06T14:26:41Z

Please add class documentation to all classes. E.g. it should be easily possible to see what the difference between RawConnection and Connection is.

bantu · 2014-03-06T14:32:05Z

apps/files_external/3rdparty/smb/rawconnection.php

+	 * $pipes[0] holds STDIN for smbclient
+	 * $pipes[1] holds STDOUT for smbclient
+	 */
+	private $pipes;


Shouldn't these be available to subclasses? Personally, I think that the number of proper use-cases of private is just as limited as the number of use-cases for static.

bantu · 2014-03-06T14:37:22Z

A PHP Stream Wrapper for SMB would be pretty awesome. :-)

PVince81 · 2014-03-26T14:37:30Z

@icewind1991 I started testing this, see some failing results here #4770 (comment)

PVince81 · 2014-03-26T14:40:16Z

I'd say better rebase this first, it seems some issues might be caused by stuff that was fixed already on master.

scrutinizer-notifier · 2014-04-07T12:11:12Z

A new inspection was created.

DeepDiver1975 · 2014-04-07T12:27:42Z

Do we really want to continue with the approach to fork smbclient processes?

Even with a much better looking code base the basic mechanism remains - I'd prefer to have a native php module which uses samba libs to talk to smb/cifs shares directly.

@karlitschek @icewind1991 @PVince81

bantu · 2014-04-07T12:31:21Z

@DeepDiver1975 I think improving the existing approach using smbclient via exec is an acceptable intermediate step.

PVince81 · 2014-04-07T12:44:14Z

One advantage I saw here is that the smbclient process is only started once, and its internal CLI is driven by @icewind1991 's code which should improve performance a bit as we don't need to restart the process for every command (stat, etc...)

I'm still a bit afraid that this big change causes regressions.

icewind1991 · 2014-04-07T12:55:49Z

I agree that having a native php module would be the optimal solution but that wont be available in all situations

ghost · 2014-04-07T13:41:33Z

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/4109/

DeepDiver1975 · 2014-04-07T14:00:13Z

that wont be available in all situations

true - but installations not capable of installing a custom/self-built php module generally don't need access to smb shares

bantu · 2014-04-20T17:28:11Z

apps/files_external/3rdparty/smb/Icewind/SMB/RawConnection.php

+	 * @return string
+	 */
+	public function read() {
+		return trim(fgets($this->pipes[1]));


Hmm. trim() on binary data?

icewind1991 · 2014-08-27T15:51:48Z

superseded by #10673

icewind1991 added 8 commits June 17, 2013 16:16

new smb storage backend

bc276ae

Merge branch 'master' into smb

a85d38d

smb: fix stat behaviour

aad2304

disable external storage tests

a77119f

merge master into smb

37f4c55

add smb wrapper to 3rdparty

88e5712

smb: fix share mtime

a73d058

use ===

8a8110b

fix accidental comment

1db7e7b

DeepDiver1975 reviewed Sep 9, 2013
View reviewed changes

fixing typo

1770fdc

merge master into smb

5141028

PVince81 reviewed Nov 28, 2013
View reviewed changes

icewind1991 added 5 commits December 17, 2013 14:19

Merge branch 'master' into smb

5c4eb85

Conflicts: apps/files_external/3rdparty/smb4php/smb.php apps/files_external/lib/smb.php

Update SMB streamwrapper

5812a5b

Fix rename not returning a result, unlink not working on folders and …

8631534

…rename not overwriting

Merge branch 'smb' of github.com:owncloud/core into smb

bd781d7

check if the file exists for isReadable and isUpdatable

8079a9c

icewind1991 added 2 commits February 25, 2014 15:11

Merge branch 'master' into smb

a674726

update smb library

80f4e68

PVince81 mentioned this pull request Mar 6, 2014

Improving ext storage with streaming #5949

Closed

10 tasks

bantu reviewed Mar 6, 2014
View reviewed changes

icewind1991 added 3 commits March 26, 2014 16:22

merge master into smb

48e8e8a

merge master into smb

8cd05d7

use PSR style autoloading for smb

5baec0f

bantu reviewed Apr 20, 2014
View reviewed changes

RobinMcCorkell added the app:files_external label Aug 26, 2014

icewind1991 closed this Aug 27, 2014

MorrisJobke deleted the smb branch January 31, 2015 08:40

lock bot locked as resolved and limited conversation to collaborators Aug 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new SMB storage backend #4770

new SMB storage backend #4770

icewind1991 commented Sep 8, 2013

ghost commented Sep 8, 2013

ghost commented Sep 8, 2013

DeepDiver1975 Sep 9, 2013

DeepDiver1975 commented Sep 9, 2013

ghost commented Sep 9, 2013

itn3rd77 commented Sep 17, 2013

DeepDiver1975 commented Oct 9, 2013

PVince81 commented Nov 19, 2013

icewind1991 commented Nov 19, 2013

PVince81 commented Nov 20, 2013

PVince81 commented Nov 21, 2013

PVince81 Nov 28, 2013

icewind1991 commented Dec 17, 2013

scrutinizer-notifier commented Feb 25, 2014

ghost commented Feb 25, 2014

bantu Mar 6, 2014

bantu commented Mar 6, 2014

bantu Mar 6, 2014

bantu commented Mar 6, 2014

PVince81 commented Mar 26, 2014

PVince81 commented Mar 26, 2014

scrutinizer-notifier commented Apr 7, 2014

DeepDiver1975 commented Apr 7, 2014

bantu commented Apr 7, 2014

PVince81 commented Apr 7, 2014

icewind1991 commented Apr 7, 2014

ghost commented Apr 7, 2014

DeepDiver1975 commented Apr 7, 2014

bantu Apr 20, 2014

icewind1991 commented Aug 27, 2014


		require_once 'errors.php';

		spl_autoload_register(function ($class) {

new SMB storage backend #4770

new SMB storage backend #4770

Conversation

icewind1991 commented Sep 8, 2013

ghost commented Sep 8, 2013

ghost commented Sep 8, 2013

DeepDiver1975 Sep 9, 2013

Choose a reason for hiding this comment

DeepDiver1975 commented Sep 9, 2013

ghost commented Sep 9, 2013

itn3rd77 commented Sep 17, 2013

DeepDiver1975 commented Oct 9, 2013

PVince81 commented Nov 19, 2013

icewind1991 commented Nov 19, 2013

PVince81 commented Nov 20, 2013

PVince81 commented Nov 21, 2013

PVince81 Nov 28, 2013

Choose a reason for hiding this comment

icewind1991 commented Dec 17, 2013

scrutinizer-notifier commented Feb 25, 2014

ghost commented Feb 25, 2014

bantu Mar 6, 2014

Choose a reason for hiding this comment

bantu commented Mar 6, 2014

bantu Mar 6, 2014

Choose a reason for hiding this comment

bantu commented Mar 6, 2014

PVince81 commented Mar 26, 2014

PVince81 commented Mar 26, 2014

scrutinizer-notifier commented Apr 7, 2014

DeepDiver1975 commented Apr 7, 2014

bantu commented Apr 7, 2014

PVince81 commented Apr 7, 2014

icewind1991 commented Apr 7, 2014

ghost commented Apr 7, 2014

DeepDiver1975 commented Apr 7, 2014

bantu Apr 20, 2014

Choose a reason for hiding this comment

icewind1991 commented Aug 27, 2014