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 5 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: 2 additions & 0 deletions ByteSource.h
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ struct SourceMetaData {
FileAllocationStatus allocationStatus{NOT_EXISTS};
/// if there is a size mismatch, this is the previous sequence id
int64_t prevSeqId{0};
/// file permission.
int64_t permission{512};
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be int32_t.

/// If true, files are read using O_DIRECT or F_NOCACHE
bool directReads{false};
/// File descriptor. If this is not -1, then wdt uses this to read
Expand Down
2 changes: 1 addition & 1 deletion CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ cmake_minimum_required(VERSION 3.2)
# There is no C per se in WDT but if you use CXX only here many checks fail
# Version is Major.Minor.YYMMDDX for up to 10 releases per day (X from 0 to 9)
# Minor currently is also the protocol version - has to match with Protocol.cpp
project("WDT" LANGUAGES C CXX VERSION 1.29.1703251)
project("WDT" LANGUAGES C CXX VERSION 1.30.1703270)
Copy link
Contributor

Choose a reason for hiding this comment

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

You will probably have to change the version to 31, as I am using 30 for an encryption fix.


# On MacOS this requires the latest (master) CMake (and/or CMake 3.1.1/3.2)
# WDT itself works fine with C++11 (gcc 4.8 for instance) but more recent folly
Expand Down
7 changes: 7 additions & 0 deletions Protocol.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ const int Protocol::INCREMENTAL_TAG_VERIFICATION_VERSION = 25;
const int Protocol::DELETE_CMD_VERSION = 26;
const int Protocol::VARINT_CHANGE = 27;
const int Protocol::HEART_BEAT_VERSION = 29;
const int Protocol::KEEP_PERMISSION = 30;
Copy link
Contributor

Choose a reason for hiding this comment

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

PRESERVE_PERMISSION


/* All methods of Protocol class are static (functions) */

Expand Down Expand Up @@ -148,6 +149,9 @@ bool Protocol::encodeHeader(int senderProtocolVersion, char *dest, int64_t &off,
encodeVarI64C(dest, umax, off, blockDetails.dataSize) &&
encodeVarI64C(dest, umax, off, blockDetails.offset) &&
encodeVarI64C(dest, umax, off, blockDetails.fileSize);
if (ok && senderProtocolVersion >= KEEP_PERMISSION) {
ok = encodeVarI64C(dest, umax, off, blockDetails.permission);
}
if (ok && senderProtocolVersion >= HEADER_FLAG_AND_PREV_SEQ_ID_VERSION) {
uint8_t flags = blockDetails.allocationStatus;
if (off >= max) {
Expand Down Expand Up @@ -178,6 +182,9 @@ bool Protocol::decodeHeader(int receiverProtocolVersion, char *src,
decodeInt64C(br, blockDetails.dataSize) &&
decodeInt64C(br, blockDetails.offset) &&
decodeInt64C(br, blockDetails.fileSize);
if (ok && receiverProtocolVersion >= KEEP_PERMISSION) {
ok = decodeInt64C(br, blockDetails.permission);
}
if (ok && receiverProtocolVersion >= HEADER_FLAG_AND_PREV_SEQ_ID_VERSION) {
if (br.empty()) {
WLOG(ERROR) << "Invalid (too short) input len " << max << " at offset "
Expand Down
12 changes: 8 additions & 4 deletions Protocol.h
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,8 @@ struct BlockDetails {
FileAllocationStatus allocationStatus{NOT_EXISTS};
/// seq-id of previous transfer, only valid if there is a size mismatch
int64_t prevSeqId{0};
/// file permission in decimal.
int64_t permission{512};
Copy link
Contributor

Choose a reason for hiding this comment

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

int32_t

};

/// structure representing settings cmd
Expand Down Expand Up @@ -270,6 +272,8 @@ class Protocol {
static const int VARINT_CHANGE;
/// version from which heart-beat was introduced
static const int HEART_BEAT_VERSION;
/// version from which file permission is kept during transfer
static const int KEEP_PERMISSION;

/// Both version, magic number and command byte
enum CMD_MAGIC {
Expand Down Expand Up @@ -298,10 +302,10 @@ class Protocol {

/// Max size of sender or receiver id
static constexpr int64_t kMaxTransferIdLength = 50;
/// 1 byte for cmd, 2 bytes for file-name length, Max size of filename, 4
/// variants(seq-id, data-size, offset, file-size), 1 byte for flag, 10 bytes
/// prev seq-id
static constexpr int64_t kMaxHeader = 1 + 2 + PATH_MAX + 4 * 10 + 1 + 10;
/// 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 + 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
1 change: 1 addition & 0 deletions SenderThread.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,7 @@ TransferStats SenderThread::sendOneByteSource(
blockDetails.dataSize = expectedSize;
blockDetails.allocationStatus = metadata.allocationStatus;
blockDetails.prevSeqId = metadata.prevSeqId;
blockDetails.permission = metadata.permission;
Protocol::encodeHeader(wdtParent_->getProtocolVersion(), headerBuf, off,
Protocol::kMaxHeader, blockDetails);
int16_t littleEndianOff = folly::Endian::little((int16_t)off);
Expand Down
6 changes: 3 additions & 3 deletions WdtConfig.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,10 @@
#include <fcntl.h>

#define WDT_VERSION_MAJOR 1
#define WDT_VERSION_MINOR 29
#define WDT_VERSION_BUILD 1703251
#define WDT_VERSION_MINOR 30
#define WDT_VERSION_BUILD 1703270
// Add -fbcode to version str
#define WDT_VERSION_STR "1.29.1703251-fbcode"
#define WDT_VERSION_STR "1.30.1703270-fbcode"
// Tie minor and proto version
#define WDT_PROTOCOL_VERSION WDT_VERSION_MINOR

Expand Down
6 changes: 6 additions & 0 deletions WdtTransferRequest.h
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,12 @@ struct WdtFileInfo {
/// Whether read should be done using o_direct. If fd is set, this flag will
/// be set automatically to match the fd open mode
bool directReads{false};
/// File permission in decimal. When smaller than oct777 (dec512), the
/// permission will be passed during transfer.
int64_t permission{512};
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this check against 512? Can't the default value be 0 and wdt always populates it to the right value in DirectorySourceQueue?

/// Constructor for file info with name, size, odirect request and permission
WdtFileInfo(const std::string& name,
int64_t size, bool directReads, int64_t perm);
/// Constructor for file info with name, size and odirect request
WdtFileInfo(const std::string& name, int64_t size, bool directReads);
/**
Expand Down
32 changes: 18 additions & 14 deletions test/ProtocolTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -25,20 +25,22 @@ void testHeader() {
bd.dataSize = 3;
bd.offset = 4;
bd.fileSize = 10;
bd.permission = 2;
bd.allocationStatus = EXISTS_CORRECT_SIZE;

char buf[128];
int64_t off = 0;
Protocol::encodeHeader(Protocol::HEADER_FLAG_AND_PREV_SEQ_ID_VERSION, buf,
Protocol::encodeHeader(Protocol::KEEP_PERMISSION, buf,
off, sizeof(buf), bd);
EXPECT_EQ(off,
bd.fileName.size() + 1 + 1 + 1 + 1 + 1 +
1); // 1 byte variant for seqId, len, size, offset and filesize
bd.fileName.size() + 1 + 1 + 1 + 1 + 1 + 1 +
1); // 1 byte variant for seqId, len, size, offset, filesize
// and permission
BlockDetails nbd;
int64_t noff = 0;
bool success =
Protocol::decodeHeader(Protocol::HEADER_FLAG_AND_PREV_SEQ_ID_VERSION, buf,
noff, sizeof(buf), nbd);
Protocol::decodeHeader(Protocol::KEEP_PERMISSION, buf, noff, sizeof(buf),
nbd);
EXPECT_TRUE(success);
EXPECT_EQ(noff, off);
EXPECT_EQ(nbd.fileName, bd.fileName);
Expand All @@ -47,17 +49,18 @@ void testHeader() {
EXPECT_EQ(nbd.offset, bd.offset);
EXPECT_EQ(nbd.dataSize, bd.dataSize);
EXPECT_EQ(nbd.allocationStatus, bd.allocationStatus);
EXPECT_EQ(nbd.permission, bd.permission);
noff = 0;
// exact size:
success = Protocol::decodeHeader(
Protocol::HEADER_FLAG_AND_PREV_SEQ_ID_VERSION, buf, noff, off, nbd);
Protocol::KEEP_PERMISSION, buf, noff, off, nbd);
EXPECT_TRUE(success);

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

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

off = 0;
Protocol::encodeHeader(Protocol::HEADER_FLAG_AND_PREV_SEQ_ID_VERSION, buf,
off, sizeof(buf), bd);
Protocol::encodeHeader(Protocol::KEEP_PERMISSION, buf, off, sizeof(buf), bd);
EXPECT_EQ(off,
bd.fileName.size() + 1 + 1 + 6 + 1 + 2 + 1 +
1); // 1 byte variant for id len and size
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::HEADER_FLAG_AND_PREV_SEQ_ID_VERSION, buf,
noff, sizeof(buf), nbd);
Protocol::decodeHeader(Protocol::KEEP_PERMISSION, buf, noff, sizeof(buf),
nbd);
EXPECT_TRUE(success);
EXPECT_EQ(noff, off);
EXPECT_EQ(nbd.fileName, bd.fileName);
EXPECT_EQ(nbd.seqId, bd.seqId);
EXPECT_EQ(nbd.fileSize, bd.fileSize);
EXPECT_EQ(nbd.offset, bd.offset);
EXPECT_EQ(nbd.dataSize, bd.dataSize);
EXPECT_EQ(nbd.permission, bd.permission);
EXPECT_EQ(nbd.allocationStatus, bd.allocationStatus);
EXPECT_EQ(nbd.prevSeqId, bd.prevSeqId);
WLOG(INFO) << "got size of " << nbd.dataSize;
// too short for size encoding:
noff = 0;
success = Protocol::decodeHeader(
Protocol::HEADER_FLAG_AND_PREV_SEQ_ID_VERSION, buf, noff, off - 2, nbd);
Protocol::KEEP_PERMISSION, buf, noff, off - 2, nbd);
EXPECT_FALSE(success);
}

Expand Down
8 changes: 7 additions & 1 deletion test/wdt_e2e_simple_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,13 @@ 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
(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 )

(cd $DIR; diff -u src.perm dst.perm)
STATUS=$?
fi

if [ $WDT_TEST_SYMLINKS -eq 1 ]; then
echo "Verifying for run with follow_symlinks"
Expand All @@ -150,9 +156,9 @@ if [ $DO_VERIFY -eq 1 ] ; then
> ../src_symlinks.md5s )
(cd $DIR/dst_symlinks ; ( find . -type f -print0 | xargs -0 $MD5SUM \
| sort ) > ../dst_symlinks.md5s )

echo "Should be no diff"
(cd $DIR; diff -u src_symlinks.md5s dst_symlinks.md5s)

SYMLINK_STATUS=$?
if [ $STATUS -eq 0 ] ; then
STATUS=$SYMLINK_STATUS
Expand Down
11 changes: 10 additions & 1 deletion util/DirectorySourceQueue.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,12 @@ 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, int64_t perm)
: WdtFileInfo(name, size, doDirectReads){
permission = perm;
}

WdtFileInfo::WdtFileInfo(int fd, int64_t size, const string &name)
: WdtFileInfo(name, size, false) {
this->fd = fd;
Expand Down Expand Up @@ -360,7 +366,9 @@ bool DirectorySourceQueue::explore() {
!std::regex_match(newRelativePath, includeRegex)) {
continue;
}
WdtFileInfo fileInfo(newRelativePath, fileStat.st_size, directReads_);
int perm = fileStat.st_mode & (S_IRWXU | S_IRWXG | S_IRWXO);
Copy link
Contributor

Choose a reason for hiding this comment

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

We need this logic also in enqueueFiles() method. If the users provide a list of files, we use that method to populate the queue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh my previous understanding was: we allow the user to specify a permission, which will be used during createFile. If no valid permission is specified in the request, we will create a 644. (That's why my default permission value is 512. Cuz 0 is still a theoretically valid permission) But I think you are right. It is better to always read the permission from the given file.

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: const int perm

WdtFileInfo fileInfo(newRelativePath,
fileStat.st_size, directReads_, perm);
createIntoQueue(newFullPath, fileInfo);
continue;
}
Expand Down Expand Up @@ -440,6 +448,7 @@ void DirectorySourceQueue::createIntoQueue(const string &fullPath,
metadata->fd = fileInfo.fd;
metadata->directReads = fileInfo.directReads;
metadata->size = fileInfo.fileSize;
metadata->permission = fileInfo.permission;
if ((openFilesDuringDiscovery_ != 0) && (metadata->fd < 0)) {
metadata->fd =
FileUtil::openForRead(*threadCtx_, fullPath, metadata->directReads);
Expand Down
18 changes: 14 additions & 4 deletions util/FileCreator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +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);
fd = createFile(threadCtx,
blockDetails->fileName, blockDetails->permission);
} else {
fd = openExistingFile(threadCtx, blockDetails->fileName);
}
Expand Down Expand Up @@ -197,7 +198,16 @@ int FileCreator::openExistingFile(ThreadCtx &threadCtx,
return res;
}

int FileCreator::createFile(ThreadCtx &threadCtx, const string &relPathStr) {
int openWithPerm(const char *path, int oFlags, int perm) {
if (perm >= 512) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure we need this check against 512. Just set the default value of BlockDetails::permission to 0644.

// no permission specified
return open(path, oFlags, 0644);
}
return open(path, oFlags, perm);
}

int FileCreator::createFile(ThreadCtx &threadCtx,
const string &relPathStr, int64_t permission) {
CHECK(!relPathStr.empty());
CHECK(relPathStr[0] != '/');
CHECK(relPathStr.back() != '/');
Expand Down Expand Up @@ -255,7 +265,7 @@ int FileCreator::createFile(ThreadCtx &threadCtx, const string &relPathStr) {
int res;
{
PerfStatCollector statCollector(threadCtx, PerfStatReport::FILE_OPEN);
res = open(path.c_str(), openFlags, 0644);
res = openWithPerm(path.c_str(), openFlags, permission);
}
if (res < 0) {
if (dir.empty()) {
Expand All @@ -276,7 +286,7 @@ int FileCreator::createFile(ThreadCtx &threadCtx, const string &relPathStr) {
}
{
PerfStatCollector statCollector(threadCtx, PerfStatReport::FILE_OPEN);
res = open(path.c_str(), openFlags, 0644);
res = openWithPerm(path.c_str(), openFlags, permission);
}
if (res < 0) {
WPLOG(ERROR) << "failed creating file " << path;
Expand Down
3 changes: 2 additions & 1 deletion util/FileCreator.h
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,8 @@ class FileCreator {
*
* @return file descriptor or -1 on error
*/
int createFile(ThreadCtx &threadCtx, const std::string &relPath);
int createFile(ThreadCtx &threadCtx,
const std::string &relPath, int64_t permission);
Copy link
Contributor

Choose a reason for hiding this comment

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

int32_t permission

/**
* Open existing file
*/
Expand Down