[Stable9] smbfixes #25817

Merged
merged 15 commits into from Aug 26, 2016

Projects

None yet

6 participants

@butonic
Member
butonic commented Aug 16, 2016

forward port of some of the smb fixes that went into 8.2

cc @dercorn

@butonic butonic added this to the 9.0.5 milestone Aug 16, 2016
@mention-bot

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

@jvillafanez jvillafanez commented on an outdated diff Aug 19, 2016
apps/files_external/lib/smb.php
$path = $this->buildPath($path);
if (!isset($this->statCache[$path])) {
- $this->log("stat fetching '{$this->root}$path'");
- $this->statCache[$path] = $this->share->stat($path);
+ try {
+ $this->log("stat fetching '{$this->root}|$path'");
+ $this->statCache[$path] = $this->share->stat($path);
+ if ($this->remoteIsShare() && $this->isRootDir($path)) {
+ // make root never hidden, may happen when accessing a shared drive (mode is 22)
+ // FIXME reuse original fileinfo mode but remove hidden flag
@jvillafanez
jvillafanez Aug 19, 2016 Contributor

Maybe we should add a log here. We don't overwrite things often so debugging this could be confusing because we expect no modifications.

@jvillafanez jvillafanez commented on the diff Aug 19, 2016
apps/files_external/lib/smb.php
@@ -185,6 +249,45 @@ public function stat($path) {
}
/**
+ * get the best guess for the modification time of the share
+ * NOTE: modification times do not bubble up the directory tree, basically
+ * we are just guessing a time
+ *
+ * @return int the calculated mtime for the folder
+ */
+ private function shareMTime() {
@jvillafanez
jvillafanez Aug 19, 2016 Contributor

We might not need this if the stat works fine with the shares. We can keep it just in case.

@jvillafanez jvillafanez commented on the diff Aug 19, 2016
apps/files_external/lib/smb.php
@@ -223,14 +326,8 @@ public function unlink($path) {
*/
public function hasUpdated($path, $time) {
$this->log('enter: '.__FUNCTION__."($path, $time)");
- if (!$path and $this->root == '/') {
- // mtime doesn't work for shares, but giving the nature of the backend,
@jvillafanez
jvillafanez Aug 19, 2016 Contributor

Is this fixed now?

@butonic
butonic Aug 26, 2016 Member

well ... not in smbclient. and we need to work around the problem if older versions are used.

Actually, the hasUpdated stuff should be removed because we euther need to periodically scan an external storage or have a notification mechanism in place. Dynamically detecting if anything changed is only possible if the storage supports some kind of history api (IIRC only dropbox and google have a delta API) or notifications (cifs ... maybe nfs). Both should be evecuted async in the background.

Nothing for this PR.

@butonic
Member
butonic commented Aug 19, 2016

This has become a release blocker.

@butonic
Member
butonic commented Aug 22, 2016

hm, I can rename a felder just fine ... also with spaces ... investigating

@butonic
Member
butonic commented Aug 22, 2016 edited

@DeepDiver1975 is jenkins using phpsmbclient or smbclient via cli? icewind1991/SMB#52

@DeepDiver1975
Member

@DeepDiver1975 is jenkins using phpsmbclient or smbclient via cli? icewind1991/SMB#52

smbclient

@butonic
Member
butonic commented Aug 23, 2016

@DeepDiver1975 for smbclient running the testsuite locally gives me only errors and failures. debugging it I don't seem to be able to allinfo any file. Testing it on the cli with smbclient I see the same thing.

Testing with libsmbclient 57of77 tests fail. Running individual tests alone makes them pass ... WTF.

Investigating further.

@jvillafanez jvillafanez commented on an outdated diff Aug 23, 2016
apps/files_external/lib/smb.php
$path = $this->buildPath($path);
if (!isset($this->statCache[$path])) {
- $this->log("stat fetching '{$this->root}$path'");
- $this->statCache[$path] = $this->share->stat($path);
+ try {
+ $this->log("stat fetching '{$this->root}|$path'");
+ $this->statCache[$path] = $this->share->stat($path);
+ if ($this->remoteIsShare() && $this->isRootDir($path) && $this->statCache[$path]->isHidden()) {
@jvillafanez
jvillafanez Aug 23, 2016 Contributor

we might want to remove the $this->remoteIsShare() from here. If the remote subfolder isn't empty, the function is intended to return false (we aren't connecting directly to a share but to a inner folder), so in that case if the folder is hidden it won't be shown.

@butonic
Member
butonic commented Aug 23, 2016

FYI: this cannot be automatically tested when you are using any Windows desktop os because "The number of connections is limited to 20 by the software license agreement". Running individual tests works.

We are not explicitly closing / disconnecting in tearDown(). That piles up connections and after 20 tests the rest fails ...

@butonic
Member
butonic commented Aug 23, 2016 edited

great, now all of the tests are broken, but they all work on my machine.

@PVince81 PVince81 commented on the diff Aug 24, 2016
apps/files_external/tests/backends/smb.php
}
protected function tearDown() {
if ($this->instance) {
$this->instance->rmdir('');
+ // force disconnect of the client
+ unset($this->instance);
@PVince81
PVince81 Aug 24, 2016 Collaborator

maybe needs a more explicit disconnect ? If it doesn't work on CI it could be due to a different PHP version where maybe the GC works differently ?

Maybe add a method "close" on the SMB instance and use method_exists here before trying to call it ?

@butonic
butonic Aug 24, 2016 Member

oh, unsetting works around the connection limit just fine. The problem with nearly all tests failing may have been caused by how the share was set up and which version of smbclient is available.

I have shared my E: drive as 'guest', with a root of '' the testsuite fails completely when using smbclient but completes fine with lbsmbclient. The problem is that I smbclient cannot get a stat with allinfo "/path/to/file". It will always get a NT_STATUS_OBJECT_NAME_NOT_FOUND getting alt name for /path/to/file. There was a related issue regarding the root of a share: #14347

Also, it seems recent smbclient changed the output of allinfo, see icewind1991/SMB#52

remember, all these problems only arise whan using the fallback smbclient implementation.

@DeepDiver1975 we also need to test libsmbclient automatically.

@butonic
butonic Aug 24, 2016 Member

I added a fallback for failing stats thet try reading the stat info from the parent folders dirlisting. Waiting for jenkins...

@PVince81
Collaborator

@butonic are these already on stable9.1 or will they need to be forward-ported too ?

@butonic
Member
butonic commented Aug 24, 2016

need to be forward ported as I was debugging this at a customer. the code in 9.1 and master is exactly the same with the upper/lower case filenames being the exception.

@jvillafanez jvillafanez and 1 other commented on an outdated diff Aug 24, 2016
apps/files_external/lib/smb.php
+ $this->statCache[$path] = $this->share->stat($path);
+ } catch (NotFoundException $e) {
+ $this->log("stat for '$path' failed, trying to read parent dir");
+ $infos = $this->share->dir(dirname($path));
+ foreach ($infos as $fileInfo) {
+ if ($fileInfo->getName() === basename($path)) {
+ $this->statCache[$path] = $fileInfo;
+ break;
+ }
+ }
+ if (empty($this->statCache[$path])) {
+ $this->leave(__FUNCTION__, $e);
+ throw $e;
+ }
+ }
+ if ($this->remoteIsShare() && $this->isRootDir($path) && $this->statCache[$path]->isHidden()) {
@jvillafanez
jvillafanez Aug 24, 2016 Contributor

This also applies if our root path targets a remote folder, not just the share, so the $this->remoteIsShare() condition should be removed

@butonic
butonic Aug 24, 2016 Member

You mean when the actual share is eg. \\host\Users but in oc is configured as 'share'=>'\\host\User\Bob', 'root'=>'' ?

@jvillafanez
jvillafanez Aug 24, 2016 Contributor

yes, more or less. An expected configuration should be 'share' => 'Users' , 'root' => 'Bob' so when the user ask for the storage's root path ('') it's internally mapped to \\host\Users\Bob.

In that case, the remoteIsShare() should return false because we're connecting to an inner folder not to the share's root.

@jvillafanez jvillafanez commented on the diff Aug 24, 2016
apps/files_external/lib/smb.php
$path = $this->buildPath($path);
if (!isset($this->statCache[$path])) {
- $this->log("stat fetching '{$this->root}$path'");
- $this->statCache[$path] = $this->share->stat($path);
+ try {
+ $this->log("stat fetching '$path'");
+ try {
+ $this->statCache[$path] = $this->share->stat($path);
+ } catch (NotFoundException $e) {
@jvillafanez
jvillafanez Aug 24, 2016 Contributor

we might need an additional check here, I don't think missing files should go through this path.

@butonic
butonic Aug 24, 2016 Member

The problem is that smbclient allinfo returns NT_STATUS_OBJECT_NAME_NOT_FOUND getting alt name for /path/to/file even though the file is there. The output parsing mechanism then thinks the file is not found. So I have to retry with the dir() fallback.

@butonic
butonic Aug 24, 2016 Member

We can try to improve the parsing logic in https://github.com/icewind1991/SMB/blob/master/src/Parser.php#L32

unfortunately, the two cases (allinfo not working vs. file does not exist) seem to be indistinguishable:

โžœ  tests git:(stable9-smbfixes) โœ— smbclient //covu/f1 -U fred
WARNING: The "syslog" option is deprecated
Enter fred's password: 
Domain=[COVU] OS=[Windows 10 Pro 10586] Server=[Windows 10 Pro 6.3]
smb: \> ls
  .                                   D        0  Wed Aug 24 09:22:01 2016
  ..                                  D        0  Wed Aug 24 09:22:01 2016
  HHUy1G7ifSAsH                       D        0  Wed Aug 24 09:21:34 2016

        121922808 blocks of size 4096. 40499483 blocks available
smb: \> allinfo HHUy1G7ifSAsH
altname: HHUY1G~1
create_time:    Mi Aug 24 09:21:34 2016 CEST
access_time:    Mi Aug 24 09:21:34 2016 CEST
write_time:     Mi Aug 24 09:21:34 2016 CEST
change_time:    Mi Aug 24 09:21:34 2016 CEST
attributes: D (10)
smb: \> allinfo nonexistant
NT_STATUS_OBJECT_NAME_NOT_FOUND getting alt name for \nonexistant
smb: \> %                                                                                                                                                                            โžœ  tests git:(stable9-smbfixes) โœ— smbclient //covu/group -U fred
WARNING: The "syslog" option is deprecated
Enter fred's password: 
Domain=[COVU] OS=[Windows 10 Pro 10586] Server=[Windows 10 Pro 6.3]
smb: \> ls
  $RECYCLE.BIN                      DHS        0  Wed Jun  1 15:30:35 2016
  3oIepyiAhLvJq                       D        0  Wed Aug 24 09:52:48 2016
  fXzPnI3bIdUOU                       D        0  Wed Aug 24 09:53:00 2016
  gpTiQrVYUV52J                       D        0  Wed Aug 24 10:01:22 2016
  i1KgmTzUOfIex                       D        0  Wed Aug 24 10:03:57 2016
  jQ9wm28mxkv3H                       D        0  Wed Aug 24 09:52:55 2016
  NiVLa3AR26aV9                       D        0  Wed Aug 24 10:01:13 2016
  oc 2                                D        0  Mon Aug 22 15:13:16 2016
  oc test foo                         D        0  Mon Aug 22 14:50:19 2016
  oc-test                             D        0  Tue Aug 23 12:00:59 2016
  occlient                            D        0  Wed Jul 13 03:51:23 2016
  System Volume Information         DHS        0  Mon Feb 15 12:28:16 2016
  zVUZI1ikLPwpm                       D        0  Wed Aug 24 10:01:27 2016

        244189951 blocks of size 4096. 197284387 blocks available
smb: \> allinfo zVUZI1ikLPwpm
NT_STATUS_OBJECT_NAME_NOT_FOUND getting alt name for \zVUZI1ikLPwpm
smb: \> 
@jvillafanez
jvillafanez Aug 24, 2016 Contributor

ok, I guess it's fine for now, but we should add a storage-based switch at some point to change between getting the information through the stat or dir calls.

@butonic
Member
butonic commented Aug 24, 2016

ok, tests are coming in green again. I needed to backport icewind1991/SMB@6e544f6 which is already in master: https://github.com/owncloud/core/blob/master/apps/files_external/3rdparty/icewind/smb/src/Share.php#L398

@butonic butonic changed the title from [WIP][Stable9] smbfixes to [Stable9] smbfixes Aug 24, 2016
@butonic
Member
butonic commented Aug 24, 2016

@jvillafanez I also added the smbclient vs libsmbcliend toggle.

@all can I haz ๐Ÿ‘ pls?

@jvillafanez jvillafanez commented on the diff Aug 24, 2016
apps/files_external/lib/smb.php
}
+ private function removeFromCache($path) {
+ $path = trim($path, '/');
+ // TODO The CappedCache does not really clear by prefix. It just clears all.
+ //$this->dirCache->clear($path);
@jvillafanez
jvillafanez Aug 24, 2016 Contributor

โš ๏ธ commented code!

@butonic
butonic Aug 25, 2016 Member

yes, I left them in to hint at what was cached in 8.2. But performance improvements can be done in a separate PR.

@jvillafanez
Contributor

My only concern is that the getFileInfo function has gotten big. I'm not sure if it's possible to simplify the function and reduce the number of lines just by splitting it into smaller functions.

Anyway ๐Ÿ‘ unless @owncloud/qa says otherwise.

@PVince81
Collaborator

@butonic had a quick look and it looks fine.

@butonic please answer this comment https://github.com/owncloud/core/pull/25817/files#r75436102

Then once Jenkins is ressurected, if the tests pass then ๐Ÿ‘

@butonic
Member
butonic commented Aug 26, 2016

all tests pass, two thumbs, all comments answered.

@butonic butonic merged commit 72fa98b into stable9 Aug 26, 2016

3 checks passed

continuous-integration/jenkins/pr This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
@butonic
Member
butonic commented Aug 26, 2016

will port forward now

@PVince81 PVince81 deleted the stable9-smbfixes branch Jan 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment