Skip to content

[Stable9] smbfixes #25817

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

Merged
butonic merged 15 commits into from
Aug 26, 2016
Merged

[Stable9] smbfixes #25817

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
61c9a06
stat for root of a share does not work
butonic Aug 16, 2016
e0c8696
don't list hidden files
butonic Aug 16, 2016
baec3a0
formatInfo should include type
butonic Aug 16, 2016
f4051a1
overwrite target on rename
butonic Aug 16, 2016
5d11696
use stat to determine has updated
butonic Aug 16, 2016
b53de63
stat now handles mtime for root shares, simplify hasUpdated
butonic Aug 16, 2016
f95fc02
move share check to getFileInfo
butonic Aug 18, 2016
2565eac
always try stat, fake if forbidden, actually return the fileinfo array
butonic Aug 18, 2016
717c521
share root is always readable
butonic Aug 18, 2016
d253b6d
do not overwrite stat cache with plain array
butonic Aug 22, 2016
1ac6f4f
fix test execution due to connection limit
butonic Aug 23, 2016
1ef2059
add dir based stat fallback
butonic Aug 24, 2016
93229e7
fix return values for smbclient backend
butonic Aug 24, 2016
185ce22
allow unhiding root folder in subfolder of share
butonic Aug 24, 2016
ebf8be1
trust libsmbclient on stat, only do dir fallback for smbclient
butonic Aug 24, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion apps/files_external/3rdparty/icewind/smb/src/Share.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -370,7 +370,7 @@ protected function execute($command) {
* @return bool
*/
protected function parseOutput($lines, $path = '') {
$this->parser->checkForError($lines, $path);
return $this->parser->checkForError($lines, $path);
}

/**
Expand Down
187 changes: 159 additions & 28 deletions apps/files_external/lib/smb.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,15 @@

namespace OC\Files\Storage;

use Icewind\SMB\Exception\AlreadyExistsException;
use Icewind\SMB\Exception\ConnectException;
use Icewind\SMB\Exception\Exception;
use Icewind\SMB\Exception\ForbiddenException;
use Icewind\SMB\Exception\NotFoundException;
use Icewind\SMB\FileInfo;
use Icewind\SMB\NativeServer;
use Icewind\SMB\Server;
use Icewind\SMB\Share;
use Icewind\Streams\CallbackWrapper;
use Icewind\Streams\IteratorDirectory;
use OC\Cache\CappedMemoryCache;
Expand Down Expand Up @@ -120,24 +123,66 @@ protected function buildPath($path) {
* @param string $path
* @return \Icewind\SMB\IFileInfo
* @throws StorageNotAvailableException
* @throws ForbiddenException
* @throws NotFoundException
*/
protected function getFileInfo($path) {
$this->log('enter: '.__FUNCTION__."($path)");
try {
$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) {
Copy link
Member

@jvillafanez jvillafanez Aug 24, 2016

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@butonic butonic Aug 24, 2016

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@butonic butonic Aug 24, 2016

Choose a reason for hiding this comment

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

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: \>

Copy link
Member

@jvillafanez jvillafanez Aug 24, 2016

Choose a reason for hiding this comment

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

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.

if ($this->share instanceof Share) {
// smbclient may have problems with the allinfo cmd
$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;
}
} else {
// trust the results of libsmb
$this->leave(__FUNCTION__, $e);
throw $e;
}
}
if ($this->isRootDir($path) && $this->statCache[$path]->isHidden()) {
$this->log("unhiding stat for '$path'");
// make root never hidden, may happen when accessing a shared drive (mode is 22, archived and readonly - neither is true ... whatever)
if ($this->statCache[$path]->isReadOnly()) {
$mode = FileInfo::MODE_DIRECTORY & FileInfo::MODE_READONLY;
} else {
$mode = FileInfo::MODE_DIRECTORY;
}
$this->statCache[$path] = new FileInfo($path, '', 0, $this->statCache[$path]->getMTime(), $mode);
}
} catch (ConnectException $e) {
$ex = new StorageNotAvailableException(
$e->getMessage(), $e->getCode(), $e);
$this->leave(__FUNCTION__, $ex);
throw $ex;
} catch (ForbiddenException $e) {
if ($this->remoteIsShare() && $this->isRootDir($path)) { //mtime may not work for share root
$this->log("faking stat for forbidden '$path'");
$this->statCache[$path] = new FileInfo($path, '', 0, $this->shareMTime(), FileInfo::MODE_DIRECTORY);
} else {
$this->leave(__FUNCTION__, $e);
throw $e;
}
}
} else {
$this->log("stat cache hit for '$path'");
}
$result = $this->statCache[$path];
} catch (ConnectException $e) {
$ex = new StorageNotAvailableException(
$e->getMessage(), $e->getCode(), $e);
$this->leave(__FUNCTION__, $ex);
throw $ex;
}
return $this->leave(__FUNCTION__, $result);
}

Expand All @@ -150,9 +195,18 @@ protected function getFolderContents($path) {
$this->log('enter: '.__FUNCTION__."($path)");
try {
$path = $this->buildPath($path);
$result = $this->share->dir($path);
foreach ($result as $file) {
$this->statCache[$path . '/' . $file->getName()] = $file;
$result = [];
$children = $this->share->dir($path);
foreach ($children as $fileInfo) {
// check if the file is readable before adding it to the list
// can't use "isReadable" function here, use smb internals instead
if ($fileInfo->isHidden()) {
$this->log("{$fileInfo->getName()} isn't readable, skipping", Util::DEBUG);
} else {
$result[] = $fileInfo;
//remember entry so we can answer file_exists and filetype without a full stat
$this->statCache[$path . '/' . $fileInfo->getName()] = $fileInfo;
}
}
} catch (ConnectException $e) {
$ex = new StorageNotAvailableException(
Expand All @@ -168,12 +222,51 @@ protected function getFolderContents($path) {
* @return array
*/
protected function formatInfo($info) {
return array(
$result = [
'size' => $info->getSize(),
'mtime' => $info->getMTime()
);
'mtime' => $info->getMTime(),
Copy link
Member

@jvillafanez jvillafanez Aug 19, 2016

Choose a reason for hiding this comment

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

Is this fixed now?

Copy link
Member Author

@butonic butonic Aug 26, 2016

Choose a reason for hiding this comment

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

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.

];
if ($info->isDirectory()) {
$result['type'] = 'dir';
} else {
$result['type'] = 'file';
}
return $result;
}

/**
* Rename the files
*
* @param string $source the old name of the path
* @param string $target the new name of the path
* @return bool true if the rename is successful, false otherwise
*/
public function rename($source, $target) {
$this->log("enter: rename('$source', '$target')", Util::DEBUG);
try {
$result = $this->share->rename($this->root . $source, $this->root . $target);
$this->removeFromCache($this->root . $source);
$this->removeFromCache($this->root . $target);
} catch (AlreadyExistsException $e) {
$this->unlink($target);
$result = $this->share->rename($this->root . $source, $this->root . $target);
$this->removeFromCache($this->root . $source);
$this->removeFromCache($this->root . $target);
$this->swallow(__FUNCTION__, $e);
} catch (\Exception $e) {
$this->swallow(__FUNCTION__, $e);
$result = false;
}
return $this->leave(__FUNCTION__, $result);
}

private function removeFromCache($path) {
$path = trim($path, '/');
// TODO The CappedCache does not really clear by prefix. It just clears all.
//$this->dirCache->clear($path);
Copy link
Member

@jvillafanez jvillafanez Aug 24, 2016

Choose a reason for hiding this comment

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

⚠️ commented code!

Copy link
Member Author

@butonic butonic Aug 25, 2016

Choose a reason for hiding this comment

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

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

$this->statCache->clear($path);
//$this->xattrCache->clear($path);
}
/**
* @param string $path
* @return array
Expand All @@ -184,6 +277,45 @@ public function stat($path) {
return $this->leave(__FUNCTION__, $result);
}

/**
* 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() {
Copy link
Member

@jvillafanez jvillafanez Aug 19, 2016

Choose a reason for hiding this comment

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

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

$this->log('enter: '.__FUNCTION__, Util::DEBUG);
$files = $this->share->dir($this->root);
$result = 0;
foreach ($files as $fileInfo) {
if ($fileInfo->getMTime() > $result) {
$result = $fileInfo->getMTime();
}
}
return $this->leave(__FUNCTION__, $result);
}
/**
* Check if the path is our root dir (not the smb one)
*
* @param string $path the path
* @return bool true if it's root, false if not
*/
private function isRootDir($path) {
$this->log('enter: '.__FUNCTION__."($path)", Util::DEBUG);
$result = $path === '' || $path === '/' || $path === '.';
return $this->leave(__FUNCTION__, $result);
}
/**
* Check if our root points to a smb share
*
* @return bool true if our root points to a share false otherwise
*/
private function remoteIsShare() {
$this->log('enter: '.__FUNCTION__, Util::DEBUG);
$result = $this->share->getName() && (!$this->root || $this->root === '/');
return $this->leave(__FUNCTION__, $result);
}
/**
* @param string $path
* @return bool
Expand Down Expand Up @@ -223,14 +355,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,
// doing a full update is still just fast enough
$result = true;
} else {
$actualTime = $this->filemtime($path);
$result = $actualTime > $time;
}
$actualTime = $this->filemtime($path);
$result = $actualTime > $time;
return $this->leave(__FUNCTION__, $result);
}

Expand Down Expand Up @@ -311,7 +437,7 @@ public function rmdir($path) {
$this->log('enter: '.__FUNCTION__."($path)");
$result = false;
try {
$this->statCache = array();
$this->removeFromCache($path);
$content = $this->share->dir($this->buildPath($path));
foreach ($content as $file) {
if ($file->isDirectory()) {
Expand Down Expand Up @@ -390,8 +516,7 @@ public function mkdir($path) {
$result = false;
$path = $this->buildPath($path);
try {
$this->share->mkdir($path);
$result = true;
$result = $this->share->mkdir($path);
} catch (ConnectException $e) {
$ex = new StorageNotAvailableException(
$e->getMessage(), $e->getCode(), $e);
Expand Down Expand Up @@ -492,7 +617,6 @@ public function test() {
return $this->leave(__FUNCTION__, $result);
}


/**
* @param string $message
* @param int $level
Expand Down Expand Up @@ -530,7 +654,7 @@ private function leave($function, $result) {
.' message: '.$result->getMessage()
.' trace: '.$result->getTraceAsString(), Util::DEBUG);
} else {
Util::writeLog('wnd', "leave: $function, return ".json_encode($result), Util::DEBUG);
Util::writeLog('wnd', "leave: $function, return ".json_encode($result, true), Util::DEBUG);
}
return $result;
}
Expand All @@ -543,4 +667,11 @@ private function swallow($function, \Exception $exception) {
.' trace: '.$exception->getTraceAsString(), Util::DEBUG);
}
}

/**
* immediately close / free connection
*/
public function __destruct() {
unset($this->share);
}
}
4 changes: 3 additions & 1 deletion apps/files_external/tests/backends/smb.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,12 +47,14 @@ protected function setUp() {
}
$config['root'] .= $id; //make sure we have an new empty folder to work in
$this->instance = new \OC\Files\Storage\SMB($config);
$this->instance->mkdir('/');
$this->assertTrue($this->instance->mkdir('/'));
}

protected function tearDown() {
if ($this->instance) {
$this->instance->rmdir('');
// force disconnect of the client
unset($this->instance);
Copy link
Contributor

@PVince81 PVince81 Aug 24, 2016

Choose a reason for hiding this comment

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

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 ?

Copy link
Member Author

@butonic butonic Aug 24, 2016

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@butonic butonic Aug 24, 2016

Choose a reason for hiding this comment

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

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

}

parent::tearDown();
Expand Down