File permission transfer #155

dliang36 · 2017-03-26T06:24:58Z

Implementation and test for keeping file permission during transfer v0.1

Design decision:

Get permission from the same st_mode that is used to get file size, and store permission in each metadata entry of DirectorySourceQueue, in order to avoid calling stat another time.
Default permission is 512 (01000). When perm in WdtFileInfo is default, library behavior is not changed.

Test:

Encode and decode in Protocol
Print src and dst file permissions to file and compare in E2E

TODO:

Shall we consider dir permission?
What if blockDetails.allocationStatus == TO_BE_DELETED but receiver's file permission is owner only?
What if sender side allows only owner's read?
permission changed – only chmod the file. Don’t trans again

facebook-github-bot · 2017-03-26T06:25:23Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact cla@fb.com if you have any questions.

facebook-github-bot · 2017-03-26T15:58:51Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

facebook-github-bot · 2017-03-27T02:43:21Z

@dliang36 updated the pull request - view changes

facebook-github-bot · 2017-03-27T02:45:23Z

@dliang36 updated the pull request - view changes

uddipta

@dliang36 Awesome work!
Some minor changes needed. Other than those, things look fine.

uddipta · 2017-03-27T19:13:34Z

ByteSource.h

@@ -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};


This can be int32_t.

uddipta · 2017-03-27T19:14:04Z

Protocol.cpp

@@ -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;


PRESERVE_PERMISSION

uddipta · 2017-03-27T19:16:18Z

Protocol.h

@@ -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};


uddipta · 2017-03-27T19:18:46Z

CMakeLists.txt

@@ -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)


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

uddipta · 2017-03-27T19:25:46Z

WdtTransferRequest.h

@@ -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};


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

uddipta · 2017-03-27T19:30:05Z

util/DirectorySourceQueue.cpp

@@ -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);


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

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.

uddipta · 2017-03-27T19:31:53Z

util/FileCreator.cpp

@@ -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) {


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

uddipta · 2017-03-27T19:36:59Z

Regarding your questions, all of them are valid. But, let's try to resolve those one by one in separate diffs.

…of request if !dir; change protocol name TODO: test for !dir request

facebook-github-bot · 2017-03-29T18:04:02Z

@dliang36 updated the pull request - view changes

facebook-github-bot · 2017-03-29T18:08:57Z

@dliang36 updated the pull request - view changes

uddipta

Almost there :)

uddipta · 2017-03-30T21:57:14Z

Protocol.cpp

@@ -179,6 +183,11 @@ bool Protocol::decodeHeader(int receiverProtocolVersion, char *src,
            decodeInt64C(br, blockDetails.dataSize) &&
            decodeInt64C(br, blockDetails.offset) &&
            decodeInt64C(br, blockDetails.fileSize);
+  if (ok && receiverProtocolVersion >= PRESERVE_PERMISSION) {
+      int64_t perm;
+      ok = decodeInt64C(br, perm);


use decodeInt32

uddipta · 2017-03-30T21:59:12Z

Protocol.h

-  /// 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;
+  /// variants(seq-id, data-size, offset, file-size), 2 byte for permission, 1


Why just 2 byte? Add 10 just to be safe.

uddipta · 2017-03-30T22:01:13Z

util/DirectorySourceQueue.cpp

@@ -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);


nit: const int perm

uddipta · 2017-03-30T22:03:33Z

util/DirectorySourceQueue.cpp

      info.fileSize = fileStat.st_size;
    }
+    info.permission = fileStat.st_mode & (S_IRWXU | S_IRWXG | S_IRWXO);


Can we have a helper function for getting the permission from the mode? Don't like that we have two places with this constants.

int getPermission(int mode) {
return mode & (S_IRWXU | S_IRWXG | S_IRWXO);
}

uddipta · 2017-03-30T22:04:20Z

util/FileCreator.h

@@ -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);


int32_t permission

facebook-github-bot · 2017-03-31T20:58:49Z

@dliang36 updated the pull request - view changes

facebook-github-bot · 2017-03-31T21:01:52Z

@dliang36 updated the pull request - view changes

facebook-github-bot · 2017-03-31T21:19:38Z

@uddipta has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

jjleng · 2017-04-04T01:44:00Z

test/wdt_e2e_simple_test.sh

@@ -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 )


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

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.

we do support Mac. We run open source continuous builds here https://travis-ci.org/facebook/wdt

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.

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

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.)

Or...is this my OS specific again?

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.

jjleng · 2017-04-04T01:45:03Z

Protocol.h

+  /// 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;


No comments for the extra + 2. Where does the 2 extra bytes come from?

My neglect. I used that for permission before this commit. Should have been deleted. Thank you for pointing out.

facebook-github-bot · 2017-04-04T02:41:27Z

@dliang36 updated the pull request - view changes - changes since last import

facebook-github-bot · 2017-04-08T06:52:59Z

@dliang36 updated the pull request - view changes - changes since last import

facebook-github-bot · 2017-04-08T07:10:19Z

@dliang36 updated the pull request - view changes - changes since last import

facebook-github-bot · 2017-04-09T23:53:07Z

@uddipta has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

uddipta

there are several more header files which need to be moved to the beginning of the cpp file.
We should be able to get this diff in after that.

uddipta · 2017-04-17T21:20:04Z

ErrorCodes.cpp

 #include <folly/Conv.h>
 #include <string.h>
+#include <wdt/ErrorCodes.h>


Can you revert this change?

uddipta · 2017-04-17T22:04:38Z

ReceiverThread.cpp

 #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>


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.

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

uddipta · 2017-04-17T22:04:55Z

Reporting.cpp

 #include <folly/String.h>
 #include <wdt/Protocol.h>
+#include <wdt/Reporting.h>


this one too

uddipta · 2017-04-17T22:05:12Z

SenderThread.cpp

 #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>


another one

uddipta · 2017-04-17T22:05:32Z

Throttler.cpp

 #include <wdt/ErrorCodes.h>
+#include <wdt/Throttler.h>


this one too

uddipta · 2017-04-17T22:05:57Z

WdtOptions.cpp

 #include <glog/logging.h>
+#include <wdt/WdtOptions.h>


uddipta · 2017-04-17T22:06:07Z

WdtTransferRequest.cpp

 #include <folly/Conv.h>
 #include <folly/Range.h>
+#include <wdt/WdtTransferRequest.h>


this one too

uddipta · 2017-04-17T22:10:49Z

test/wdt_e2e_simple_test.sh

+# 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


why do we need to set this?

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.

facebook-github-bot · 2017-04-23T21:31:11Z

@dliang36 updated the pull request - view changes - changes since last import

facebook-github-bot · 2017-04-27T17:18:31Z

@dliang36 updated the pull request - view changes - changes since last import

facebook-github-bot · 2017-04-27T17:27:21Z

@dliang36 updated the pull request - view changes - changes since last import

facebook-github-bot · 2017-04-27T17:29:07Z

@dliang36 updated the pull request - view changes - changes since last import

facebook-github-bot · 2017-04-27T17:56:43Z

@uddipta has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

dliang36 added 3 commits March 22, 2017 16:18

Sender and e2e test

d08d3e9

Protocol test for permission

8e671b7

receiver side

b85b559

facebook-github-bot added the CLA Signed label Mar 26, 2017

add protocol version

93e61d3

Merge branch 'master' into master

90dafff

uddipta suggested changes Mar 27, 2017

View reviewed changes

dliang36 added 2 commits March 29, 2017 10:56

change default permission to 0644; read permission from stat instead …

b6def07

…of request if !dir; change protocol name TODO: test for !dir request

Merge branch 'master' of https://github.com/dliang36/wdt

93ed192

Merge branch 'master' into master

eed6c64

uddipta reviewed Mar 30, 2017

View reviewed changes

include set-user-ID bit | set-group-ID bit | sticky bit in permission

37ddca5

Merge branch 'master' into master

5b1af9b

jjleng reviewed Apr 4, 2017

View reviewed changes

dliang36 added 2 commits April 3, 2017 19:38

Read file tests and Protocol.h neglect correction

6a9dae2

Merge branch 'master' of https://github.com/dliang36/wdt

e3c2c6a

chmod file to test set-user-ID bit | set-group-ID bit | sticky bit

7bb17e3

clang

0125fe6

uddipta reviewed Apr 17, 2017

View reviewed changes

revert includes that cause FB lint problem

b57a4fc

more reverts to avoid FB lint problem

caf98ff

merge conflicts

ae240d9

Merge branch 'master' into master

3f889bd

facebook-github-bot closed this in 3a5b195 Apr 27, 2017

File permission transfer #155

File permission transfer #155

Conversation

dliang36 commented Mar 26, 2017

facebook-github-bot commented Mar 26, 2017

facebook-github-bot commented Mar 26, 2017

facebook-github-bot commented Mar 27, 2017

facebook-github-bot commented Mar 27, 2017

uddipta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uddipta commented Mar 27, 2017

facebook-github-bot commented Mar 29, 2017

facebook-github-bot commented Mar 29, 2017

uddipta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Mar 31, 2017

facebook-github-bot commented Mar 31, 2017

facebook-github-bot commented Mar 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dliang36 Apr 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Apr 4, 2017

facebook-github-bot commented Apr 8, 2017

facebook-github-bot commented Apr 8, 2017

facebook-github-bot commented Apr 9, 2017

uddipta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Apr 23, 2017

facebook-github-bot commented Apr 27, 2017

facebook-github-bot commented Apr 27, 2017

facebook-github-bot commented Apr 27, 2017

facebook-github-bot commented Apr 27, 2017

dliang36 Apr 5, 2017 •

edited

Loading