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.

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

File permission transfer #155

Closed
wants to merge 18 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
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 ErrorCodes.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@
* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*/
#include <wdt/ErrorCodes.h>
#include <folly/Conv.h>
#include <string.h>
#include <wdt/ErrorCodes.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you revert this change?


DEFINE_int32(wdt_double_precision, 2, "Precision while printing double");

Expand Down
2 changes: 1 addition & 1 deletion Protocol.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ bool Protocol::decodeHeader(int receiverProtocolVersion, char *src,
decodeInt64C(br, blockDetails.offset) &&
decodeInt64C(br, blockDetails.fileSize);
if (ok && receiverProtocolVersion >= PRESERVE_PERMISSION) {
ok = decodeInt32C(br, blockDetails.permission);
ok = decodeInt32C(br, blockDetails.permission);
}
if (ok && receiverProtocolVersion >= HEADER_FLAG_AND_PREV_SEQ_ID_VERSION) {
if (br.empty()) {
Expand Down
2 changes: 1 addition & 1 deletion Protocol.h
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,7 @@ class Protocol {
/// 1 byte for cmd, 2 bytes for file-name length, Max size of filename, 5
/// variants(seq-id, data-size, offset, file-size, permission), 1 byte for
/// flag, 10 bytes prev seq-id
static constexpr int64_t kMaxHeader = 1 + 2 + PATH_MAX + 5 * 10 + 2 + 1 + 10;
static constexpr int64_t kMaxHeader = 1 + 2 + PATH_MAX + 5 * 10 + 1 + 10;
/// min number of bytes that must be send to unblock receiver
static constexpr int64_t kMinBufLength = 256;
/// max size of done command encoding(1 byte for cmd, 1 for status, 10 for
Expand Down
2 changes: 1 addition & 1 deletion ReceiverThread.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,13 @@
* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*/
#include <wdt/ReceiverThread.h>
#include <folly/Bits.h>
#include <folly/Checksum.h>
#include <folly/Conv.h>
#include <folly/Memory.h>
#include <folly/ScopeGuard.h>
#include <folly/String.h>
#include <wdt/ReceiverThread.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

Revert this one too.

"The associated header file of .cpp files should be included before any other includes. (This helps catch missing header file dependencies in the .h)"

This is the lint error I'm seeing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah... this is what clang gave me... Let me revert it...

#include <wdt/util/FileWriter.h>

namespace facebook {
Expand Down
2 changes: 1 addition & 1 deletion Reporting.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@
* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*/
#include <wdt/Reporting.h>
#include <folly/String.h>
#include <wdt/Protocol.h>
#include <wdt/Reporting.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

this one too

#include <wdt/WdtOptions.h>

#include <algorithm>
Expand Down
2 changes: 1 addition & 1 deletion SenderThread.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,14 @@
* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*/
#include <wdt/SenderThread.h>
#include <folly/Bits.h>
#include <folly/Checksum.h>
#include <folly/Conv.h>
#include <folly/Memory.h>
#include <folly/String.h>
#include <sys/stat.h>
#include <wdt/Sender.h>
#include <wdt/SenderThread.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

another one

#include <wdt/util/ClientSocket.h>

namespace facebook {
Expand Down
2 changes: 1 addition & 1 deletion Throttler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@
* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*/
#include <wdt/Throttler.h>
#include <wdt/ErrorCodes.h>
#include <wdt/Throttler.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

this one too

#include <wdt/WdtOptions.h>

namespace facebook {
Expand Down
2 changes: 1 addition & 1 deletion WdtOptions.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@
* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*/
#include <wdt/WdtOptions.h>
#include <glog/logging.h>
#include <wdt/WdtOptions.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

another


namespace facebook {
namespace wdt {
Expand Down
2 changes: 1 addition & 1 deletion WdtTransferRequest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@
* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*/
#include <wdt/WdtTransferRequest.h>
#include <folly/Conv.h>
#include <folly/Range.h>
#include <wdt/WdtTransferRequest.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

this one too

#include <ctime>

using namespace std;
Expand Down
4 changes: 2 additions & 2 deletions WdtTransferRequest.h
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,8 @@ struct WdtFileInfo {
/// File permission.
int32_t permission;
/// Constructor for file info with name, size, odirect request and permission
WdtFileInfo(const std::string& name,
int64_t size, bool directReads, int32_t perm);
WdtFileInfo(const std::string& name, int64_t size, bool directReads,
int32_t perm);
/// Constructor for file info with name, size and odirect request
WdtFileInfo(const std::string& name, int64_t size, bool directReads);
/**
Expand Down
1 change: 1 addition & 0 deletions build/travis_osx.sh
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ export PATH=/usr/local/opt/openssl/bin:$CMAKE_BIN_DIR:$HOME/bin:$PATH
export LD_LIBRARY_PATH=/usr/local/opt/openssl/lib:$HOME/lib:$LD_LIBRARY_PATH
export OPENSSL_ROOT_DIR=/usr/local/opt/openssl
export CMAKE_PREFIX_PATH=$HOME
export WDT_TEST_PERMISSION=0
openssl version -a
wget https://www.cmake.org/files/v3.6/$CMAKE_BASE.tar.gz
tar xfz $CMAKE_BASE.tar.gz
Expand Down
22 changes: 22 additions & 0 deletions test/FileReaderTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,18 @@ class RandomFile {
int64_t getSize() const {
return fileSize_;
}
int32_t getPermission() {
if (permission_ == -1) {
struct stat fileStat;
if (stat(fileName_.c_str(), &fileStat) != 0) {
WLOG(ERROR) << "Error when calling stat()";
return -1;
}
permission_ = fileStat.st_mode &
(S_ISUID | S_ISGID | S_ISVTX | S_IRWXU | S_IRWXG | S_IRWXO);
}
return permission_;
}
const string& getFileName() const {
return fileName_;
}
Expand All @@ -65,6 +77,7 @@ class RandomFile {

private:
int64_t fileSize_{-1};
int32_t permission_{-1};
string fileName_;
SourceMetaData* metaData_{nullptr};
};
Expand Down Expand Up @@ -98,6 +111,10 @@ void testReadSize(int64_t fileSize, ByteSource& byteSource) {
EXPECT_EQ(totalSizeRead, fileSize);
}

void testReadPermission(int32_t permission, ByteSource& byteSource) {
EXPECT_EQ(permission, byteSource.getMetaData().permission);
}

void testFileRead(const WdtOptions& options, int64_t fileSize,
bool directReads) {
RandomFile file(fileSize);
Expand Down Expand Up @@ -158,6 +175,7 @@ TEST(FileByteSource, FILEINFO_ODIRECT) {
ThreadCtx threadCtx(options, true);
auto byteSource = Q.getNextSource(&threadCtx, code);
testReadSize(sizeToRead, *byteSource);
testReadPermission(file.getPermission(), *byteSource);
}

TEST(FileByteSource, MULTIPLEFILES_ODIRECT) {
Expand Down Expand Up @@ -191,6 +209,7 @@ TEST(FileByteSource, MULTIPLEFILES_ODIRECT) {
break;
}
testReadSize(sizeToRead, *byteSource);
testReadPermission(randFiles[fileNumber].getPermission(), *byteSource);
++fileNumber;
}
EXPECT_EQ(fileNumber, numFiles);
Expand Down Expand Up @@ -219,13 +238,16 @@ TEST(FileByteSource, MULTIPLEFILES_REGULAR) {
Q.setFileInfo(files);
Q.buildQueueSynchronously();
ErrorCode code;
int fileNumber = 0;
ThreadCtx threadCtx(options, true);
while (true) {
auto byteSource = Q.getNextSource(&threadCtx, code);
if (!byteSource) {
break;
}
testReadSize(sizeToRead, *byteSource);
testReadPermission(randFiles[fileNumber].getPermission(), *byteSource);
++fileNumber;
}
}
}
Expand Down
24 changes: 11 additions & 13 deletions test/ProtocolTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,8 @@ void testHeader() {
// and permission
BlockDetails nbd;
int64_t noff = 0;
bool success =
Protocol::decodeHeader(Protocol::PRESERVE_PERMISSION, buf, noff,
sizeof(buf), nbd);
bool success = Protocol::decodeHeader(Protocol::PRESERVE_PERMISSION, buf,
noff, sizeof(buf), nbd);
EXPECT_TRUE(success);
EXPECT_EQ(noff, off);
EXPECT_EQ(nbd.fileName, bd.fileName);
Expand All @@ -52,15 +51,15 @@ void testHeader() {
EXPECT_EQ(nbd.permission, bd.permission);
noff = 0;
// exact size:
success = Protocol::decodeHeader(
Protocol::PRESERVE_PERMISSION, buf, noff, off, nbd);
success = Protocol::decodeHeader(Protocol::PRESERVE_PERMISSION, buf, noff,
off, nbd);
EXPECT_TRUE(success);

WLOG(INFO) << "error tests, expect errors";
// too short
noff = 0;
success = Protocol::decodeHeader(
Protocol::PRESERVE_PERMISSION, buf, noff, off - 1, nbd);
success = Protocol::decodeHeader(Protocol::PRESERVE_PERMISSION, buf, noff,
off - 1, nbd);
EXPECT_FALSE(success);

// Long size:
Expand All @@ -69,7 +68,7 @@ void testHeader() {
bd.seqId = 5;
bd.offset = 3;
bd.fileSize = 128;
bd.permission = 511; // 777
bd.permission = 511; // 777
bd.allocationStatus = EXISTS_TOO_SMALL;
bd.prevSeqId = 10;

Expand All @@ -80,9 +79,8 @@ void testHeader() {
bd.fileName.size() + 1 + 1 + 6 + 1 + 2 + 1 + 1 +
2); // 1 byte variant for id len and size
noff = 0;
success =
Protocol::decodeHeader(Protocol::PRESERVE_PERMISSION, buf, noff,
sizeof(buf), nbd);
success = Protocol::decodeHeader(Protocol::PRESERVE_PERMISSION, buf, noff,
sizeof(buf), nbd);
EXPECT_TRUE(success);
EXPECT_EQ(noff, off);
EXPECT_EQ(nbd.fileName, bd.fileName);
Expand All @@ -96,8 +94,8 @@ void testHeader() {
WLOG(INFO) << "got size of " << nbd.dataSize;
// too short for size encoding:
noff = 0;
success = Protocol::decodeHeader(
Protocol::PRESERVE_PERMISSION, buf, noff, off - 2, nbd);
success = Protocol::decodeHeader(Protocol::PRESERVE_PERMISSION, buf, noff,
off - 2, nbd);
EXPECT_FALSE(success);
}

Expand Down
15 changes: 14 additions & 1 deletion test/wdt_e2e_simple_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,15 @@ if [ -z "$WDT_TEST_SYMLINKS" ] ; then
fi
echo "WDT_TEST_SYMLINKS=$WDT_TEST_SYMLINKS"

# Set WDT_TEST_PERMISSION:
# to 1 : check if permissions match
# to 0 : when "find -printf %m" does not work, i.e. on Mac
if [ -z "$WDT_TEST_PERMISSION" ] ; then
WDT_TEST_PERMISSION=1
umask 0
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need to set this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess you are asking about why umask? Some machines (including mine) have a mask by default, which prevents giving write permission to Others. If we do not umask it, the file created on the receiver side will not have the write perm even if it is a 0777 on the writer side.

fi
echo "WDT_TEST_PERMISSION=$WDT_TEST_PERMISSION"

# Verbose / to debug failure:
#WDTBIN="_bin/wdt/wdt -minloglevel 0 -v 99"
# Normal:
Expand Down Expand Up @@ -96,6 +105,10 @@ do
-directory="$DIR/src" -filename="$base.$i" -gen_size_mb="$size"
done
done

if [ $WDT_TEST_PERMISSION -eq 1 ]; then
chmod 01777 $DIR/src/inp.0009765625.1
fi
echo "done with setup"

if [ $WDT_TEST_SYMLINKS -eq 1 ]; then
Expand Down Expand Up @@ -137,7 +150,7 @@ if [ $DO_VERIFY -eq 1 ] ; then
echo "Should be no diff"
(cd $DIR; diff -u src.md5s dst.md5s)
STATUS=$?
if [ $STATUS -eq 0 ] ; then
if [ $STATUS -eq 0 ] && [ $WDT_TEST_PERMISSION -eq 1 ]; then
(cd $DIR/src ; ( find . -type f -printf %f%m\\n | sort ) > ../src.perm )
Copy link

Choose a reason for hiding this comment

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

find . -type f -printf %f%m\\n doesn't work on Mac. Can you also test files with permissions other than 644?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently within the /tmp/wdtTest_[usrname], there are several files with 664, which include ".a" and ".b" if I remember correctly. Regarding Mac... I am not sure whether we need to consider that. @uddipta Could you clarify this? Similarly I think stat() might not work on Windows.

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. we do support Mac. We run open source continuous builds here https://travis-ci.org/facebook/wdt
  2. In e2e simple test, I don't see any files having permissions other than 644.

So, we need to add a file with different permission for testing.

As Jason mentioned before, current find cmd does not work on mac. So, we can disable the test for now on Mac. You can do that using an environment variable. We can have an environment variable like SKIP_PERMISSION_TEST and set it in https://github.com/facebook/wdt/blob/master/build/travis_osx.sh.

If you have access to a mac, you can also try to make the test work for mac. Right now, find is failing for mac, but we are not checking the return code. Both, src.perm and dst.perm are empty, so the test is passing.

Also, run clangformat.sh once.

Every time you update this PR, you should see a travis build/test triggered.

Copy link
Contributor Author

@dliang36 dliang36 Apr 5, 2017

Choose a reason for hiding this comment

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

  1. OK. I will add a environment var for that. Sorry but I do not have a mac at hand...

  2. Well... there are on my laptop...
    If I just change this line https://github.com/facebook/wdt/pull/155/files#diff-a77a3101b61bf7da9963a0791dba5bb6R247 to return 0644,
    The following tests FAILED:
    10 - FileReaderTests (Failed)
    12 - WdtBasicE2E (Failed)
    19 - WdtSimpleOdirectTest (Failed)
    For 12 & 19, we have things like this:
    Should be no diff
    --- src.perm 2017-04-05 14:56:37.893425997 -0700
    +++ dst.perm 2017-04-05 14:56:37.893425997 -0700
    @@ -1,7 +1,7 @@
    -a664
    -c664
    -file1664
    -file1664
    +a644
    +c644
    +file1644
    +file1644

(Part of "ls -l" output on the src dir is here.)
screenshot from 2017-04-05 15-05-34
Or...is this my OS specific again?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is better to chmod a file to have permission like 777. Just do that for file1.

Regarding adding test for mac, we will add it later.

(cd $DIR/dst ; ( find . -type f -printf %f%m\\n | sort ) > ../dst.perm )

Expand Down
2 changes: 1 addition & 1 deletion util/ClientSocket.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@
* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*/
#include <wdt/util/ClientSocket.h>
#include <wdt/Reporting.h>
#include <wdt/util/ClientSocket.h>

#include <fcntl.h>
#include <glog/logging.h>
Expand Down
10 changes: 5 additions & 5 deletions util/DirectorySourceQueue.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,9 @@ WdtFileInfo::WdtFileInfo(const string &name, int64_t size, bool doDirectReads)
: fileName(name), fileSize(size), directReads(doDirectReads) {
}

WdtFileInfo::WdtFileInfo(const string &name,
int64_t size, bool doDirectReads, int32_t perm)
: WdtFileInfo(name, size, doDirectReads){
WdtFileInfo::WdtFileInfo(const string &name, int64_t size, bool doDirectReads,
int32_t perm)
: WdtFileInfo(name, size, doDirectReads) {
permission = perm;
}

Expand Down Expand Up @@ -372,8 +372,8 @@ bool DirectorySourceQueue::explore() {
continue;
}
const int perm = getPermission(fileStat.st_mode);
WdtFileInfo fileInfo(newRelativePath,
fileStat.st_size, directReads_, perm);
WdtFileInfo fileInfo(newRelativePath, fileStat.st_size, directReads_,
perm);
createIntoQueue(newFullPath, fileInfo);
continue;
}
Expand Down
2 changes: 1 addition & 1 deletion util/EncryptionUtils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,11 @@
* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*/
#include <wdt/util/EncryptionUtils.h>
#include <folly/Conv.h>
#include <folly/SpinLock.h>
#include <folly/String.h> // for humanify
#include <wdt/WdtTransferRequest.h> // for to/fromHex utils
#include <wdt/util/EncryptionUtils.h>

#include <openssl/crypto.h>
#include <openssl/rand.h>
Expand Down
10 changes: 5 additions & 5 deletions util/FileCreator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@
* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*/
#include <wdt/util/FileCreator.h>
#include <wdt/ErrorCodes.h>
#include <wdt/util/FileCreator.h>

#include <fcntl.h>
#include <folly/Conv.h>
Expand Down Expand Up @@ -58,8 +58,8 @@ int FileCreator::openAndSetSize(ThreadCtx &threadCtx,
const bool doCreate = (blockDetails->allocationStatus == NOT_EXISTS);
const bool isTooLarge = (blockDetails->allocationStatus == EXISTS_TOO_LARGE);
if (doCreate) {
fd = createFile(threadCtx,
blockDetails->fileName, blockDetails->permission);
fd =
createFile(threadCtx, blockDetails->fileName, blockDetails->permission);
} else {
fd = openExistingFile(threadCtx, blockDetails->fileName);
}
Expand Down Expand Up @@ -198,8 +198,8 @@ int FileCreator::openExistingFile(ThreadCtx &threadCtx,
return res;
}

int FileCreator::createFile(ThreadCtx &threadCtx,
const string &relPathStr, int32_t permission) {
int FileCreator::createFile(ThreadCtx &threadCtx, const string &relPathStr,
int32_t permission) {
CHECK(!relPathStr.empty());
CHECK(relPathStr[0] != '/');
CHECK(relPathStr.back() != '/');
Expand Down
4 changes: 2 additions & 2 deletions util/FileCreator.h
Original file line number Diff line number Diff line change
Expand Up @@ -108,8 +108,8 @@ class FileCreator {
*
* @return file descriptor or -1 on error
*/
int createFile(ThreadCtx &threadCtx,
const std::string &relPath, int32_t permission);
int createFile(ThreadCtx &threadCtx, const std::string &relPath,
int32_t permission);
/**
* Open existing file
*/
Expand Down
2 changes: 1 addition & 1 deletion util/FileWriter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@
* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*/
#include <wdt/util/FileWriter.h>
#include <wdt/util/CommonImpl.h>
#include <wdt/util/FileWriter.h>

#include <fcntl.h>
#include <gflags/gflags.h>
Expand Down
Loading