Uploaded image for project: 'Hadoop Common'
  1. Hadoop Common
  2. HADOOP-14843

Improve FsPermission symbolic parsing unit test coverage

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.7.4, 2.8.1
    • Fix Version/s: 2.9.0, 3.0.0-beta1
    • Component/s: fs
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      A user misunderstood the syntax format for the FsPermission symbolic constructor and passed the argument "-rwr" instead of "u=rw,g=r". In 2.7 and earlier this was silently misinterpreted as mode 0777 and in 2.8 it oddly became mode 0000. In either case FsPermission should have flagged "-rwr" as an invalid argument rather than silently misinterpreting it.

      1. HADOOP-14843.01.patch
        3 kB
        Bharat Viswanadham
      2. HADOOP-14843.02.patch
        3 kB
        Bharat Viswanadham
      3. HADOOP-14843.03.patch
        3 kB
        Bharat Viswanadham
      4. HADOOP-14843.04.patch
        3 kB
        Bharat Viswanadham
      5. HADOOP-14843.patch
        2 kB
        Bharat Viswanadham

        Activity

        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #12852 (See https://builds.apache.org/job/Hadoop-trunk-Commit/12852/)
        HADOOP-14843. Improve FsPermission symbolic parsing unit test coverage. (jlowe: rev 86f4d1c66c8b541465ff769e5d951305c41c715c)

        • (edit) hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/permission/TestFsPermission.java
        Show
        hudson Hudson added a comment - SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #12852 (See https://builds.apache.org/job/Hadoop-trunk-Commit/12852/ ) HADOOP-14843 . Improve FsPermission symbolic parsing unit test coverage. (jlowe: rev 86f4d1c66c8b541465ff769e5d951305c41c715c) (edit) hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/permission/TestFsPermission.java
        Hide
        jlowe Jason Lowe added a comment -

        Thanks, Bharat Viswanadham! I committed this to trunk, branch-3.0, and branch-2.

        Show
        jlowe Jason Lowe added a comment - Thanks, Bharat Viswanadham ! I committed this to trunk, branch-3.0, and branch-2.
        Hide
        jlowe Jason Lowe added a comment -

        Thanks for updating the patch!

        +1 lgtm. I'll commit this later today if there are no objections.

        Show
        jlowe Jason Lowe added a comment - Thanks for updating the patch! +1 lgtm. I'll commit this later today if there are no objections.
        Hide
        bharatviswa Bharat Viswanadham added a comment -

        Test failure is not related to this patch.

        Show
        bharatviswa Bharat Viswanadham added a comment - Test failure is not related to this patch.
        Hide
        hadoopqa Hadoop QA added a comment -
        -1 overall



        Vote Subsystem Runtime Comment
        0 reexec 0m 22s Docker mode activated.
              Prechecks
        +1 @author 0m 0s The patch does not contain any @author tags.
        +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
              trunk Compile Tests
        +1 mvninstall 13m 1s trunk passed
        +1 compile 15m 1s trunk passed
        +1 checkstyle 0m 31s trunk passed
        +1 mvnsite 0m 59s trunk passed
        +1 findbugs 1m 23s trunk passed
        +1 javadoc 0m 43s trunk passed
              Patch Compile Tests
        +1 mvninstall 0m 40s the patch passed
        +1 compile 11m 4s the patch passed
        +1 javac 11m 4s the patch passed
        +1 checkstyle 0m 37s the patch passed
        +1 mvnsite 0m 59s the patch passed
        +1 whitespace 0m 0s The patch has no whitespace issues.
        +1 findbugs 1m 26s the patch passed
        +1 javadoc 0m 46s the patch passed
              Other Tests
        -1 unit 7m 10s hadoop-common in the patch failed.
        +1 asflicense 0m 23s The patch does not generate ASF License warnings.
        56m 46s



        Reason Tests
        Failed junit tests hadoop.net.TestDNS



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:71bbb86
        JIRA Issue HADOOP-14843
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12886529/HADOOP-14843.04.patch
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux 723077ea6ede 4.4.0-43-generic #63-Ubuntu SMP Wed Oct 12 13:48:03 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
        Build tool maven
        Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
        git revision trunk / 6651cbc
        Default Java 1.8.0_144
        findbugs v3.1.0-RC1
        unit https://builds.apache.org/job/PreCommit-HADOOP-Build/13260/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
        Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/13260/testReport/
        modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
        Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/13260/console
        Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 22s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.       trunk Compile Tests +1 mvninstall 13m 1s trunk passed +1 compile 15m 1s trunk passed +1 checkstyle 0m 31s trunk passed +1 mvnsite 0m 59s trunk passed +1 findbugs 1m 23s trunk passed +1 javadoc 0m 43s trunk passed       Patch Compile Tests +1 mvninstall 0m 40s the patch passed +1 compile 11m 4s the patch passed +1 javac 11m 4s the patch passed +1 checkstyle 0m 37s the patch passed +1 mvnsite 0m 59s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 26s the patch passed +1 javadoc 0m 46s the patch passed       Other Tests -1 unit 7m 10s hadoop-common in the patch failed. +1 asflicense 0m 23s The patch does not generate ASF License warnings. 56m 46s Reason Tests Failed junit tests hadoop.net.TestDNS Subsystem Report/Notes Docker Image:yetus/hadoop:71bbb86 JIRA Issue HADOOP-14843 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12886529/HADOOP-14843.04.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 723077ea6ede 4.4.0-43-generic #63-Ubuntu SMP Wed Oct 12 13:48:03 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 6651cbc Default Java 1.8.0_144 findbugs v3.1.0-RC1 unit https://builds.apache.org/job/PreCommit-HADOOP-Build/13260/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/13260/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/13260/console Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        bharatviswa Bharat Viswanadham added a comment - - edited

        Jason Lowe Thanks for review. Updated the patch to use octal notation for missing 2 code lines.

        Adding Arpit Agarwal for reviewing the changes.

        Show
        bharatviswa Bharat Viswanadham added a comment - - edited Jason Lowe Thanks for review. Updated the patch to use octal notation for missing 2 code lines. Adding Arpit Agarwal for reviewing the changes.
        Hide
        jlowe Jason Lowe added a comment -

        Thanks for updating the patch! Looks good overall, just a readability nit for these two lines:

            assertEquals(950, new FsPermission("+rwrt").toShort());
        [...]
            assertEquals(1023, new FsPermission("+rwxt").toShort());
        

        It would be easier to read using the octal notation for the integer constants as was done in the other toShort tests above this.

        Show
        jlowe Jason Lowe added a comment - Thanks for updating the patch! Looks good overall, just a readability nit for these two lines: assertEquals(950, new FsPermission( "+rwrt" ).toShort()); [...] assertEquals(1023, new FsPermission( "+rwxt" ).toShort()); It would be easier to read using the octal notation for the integer constants as was done in the other toShort tests above this.
        Hide
        hadoopqa Hadoop QA added a comment -
        +1 overall



        Vote Subsystem Runtime Comment
        0 reexec 0m 19s Docker mode activated.
              Prechecks
        +1 @author 0m 0s The patch does not contain any @author tags.
        +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
              trunk Compile Tests
        +1 mvninstall 12m 57s trunk passed
        +1 compile 13m 41s trunk passed
        +1 checkstyle 0m 34s trunk passed
        +1 mvnsite 0m 57s trunk passed
        +1 findbugs 1m 18s trunk passed
        +1 javadoc 0m 53s trunk passed
              Patch Compile Tests
        +1 mvninstall 0m 41s the patch passed
        +1 compile 10m 23s the patch passed
        +1 javac 10m 23s the patch passed
        +1 checkstyle 0m 34s the patch passed
        +1 mvnsite 0m 52s the patch passed
        +1 whitespace 0m 0s The patch has no whitespace issues.
        +1 findbugs 1m 29s the patch passed
        +1 javadoc 0m 43s the patch passed
              Other Tests
        +1 unit 7m 34s hadoop-common in the patch passed.
        +1 asflicense 0m 27s The patch does not generate ASF License warnings.
        55m 12s



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:71bbb86
        JIRA Issue HADOOP-14843
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12885953/HADOOP-14843.03.patch
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux c70caf9b165a 4.4.0-43-generic #63-Ubuntu SMP Wed Oct 12 13:48:03 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
        Build tool maven
        Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
        git revision trunk / 52b894d
        Default Java 1.8.0_144
        findbugs v3.1.0-RC1
        Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/13201/testReport/
        modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
        Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/13201/console
        Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 19s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.       trunk Compile Tests +1 mvninstall 12m 57s trunk passed +1 compile 13m 41s trunk passed +1 checkstyle 0m 34s trunk passed +1 mvnsite 0m 57s trunk passed +1 findbugs 1m 18s trunk passed +1 javadoc 0m 53s trunk passed       Patch Compile Tests +1 mvninstall 0m 41s the patch passed +1 compile 10m 23s the patch passed +1 javac 10m 23s the patch passed +1 checkstyle 0m 34s the patch passed +1 mvnsite 0m 52s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 29s the patch passed +1 javadoc 0m 43s the patch passed       Other Tests +1 unit 7m 34s hadoop-common in the patch passed. +1 asflicense 0m 27s The patch does not generate ASF License warnings. 55m 12s Subsystem Report/Notes Docker Image:yetus/hadoop:71bbb86 JIRA Issue HADOOP-14843 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12885953/HADOOP-14843.03.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux c70caf9b165a 4.4.0-43-generic #63-Ubuntu SMP Wed Oct 12 13:48:03 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 52b894d Default Java 1.8.0_144 findbugs v3.1.0-RC1 Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/13201/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/13201/console Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        bharatviswa Bharat Viswanadham added a comment -

        Updated. Attached the patch to resolve the checkstyle issues.

        Show
        bharatviswa Bharat Viswanadham added a comment - Updated. Attached the patch to resolve the checkstyle issues.
        Hide
        hadoopqa Hadoop QA added a comment -
        -1 overall



        Vote Subsystem Runtime Comment
        0 reexec 0m 16s Docker mode activated.
              Prechecks
        +1 @author 0m 0s The patch does not contain any @author tags.
        +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
              trunk Compile Tests
        +1 mvninstall 13m 52s trunk passed
        +1 compile 14m 10s trunk passed
        +1 checkstyle 0m 36s trunk passed
        +1 mvnsite 1m 0s trunk passed
        +1 findbugs 1m 23s trunk passed
        +1 javadoc 0m 51s trunk passed
              Patch Compile Tests
        +1 mvninstall 0m 38s the patch passed
        +1 compile 11m 53s the patch passed
        +1 javac 11m 53s the patch passed
        -0 checkstyle 0m 40s hadoop-common-project/hadoop-common: The patch generated 9 new + 10 unchanged - 0 fixed = 19 total (was 10)
        +1 mvnsite 1m 0s the patch passed
        +1 whitespace 0m 0s The patch has no whitespace issues.
        +1 findbugs 1m 36s the patch passed
        +1 javadoc 0m 50s the patch passed
              Other Tests
        -1 unit 8m 46s hadoop-common in the patch failed.
        +1 asflicense 0m 30s The patch does not generate ASF License warnings.
        59m 55s



        Reason Tests
        Failed junit tests hadoop.fs.sftp.TestSFTPFileSystem
          hadoop.security.TestKDiag



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:71bbb86
        JIRA Issue HADOOP-14843
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12885930/HADOOP-14843.02.patch
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux 8cbbe3c25c1a 3.13.0-117-generic #164-Ubuntu SMP Fri Apr 7 11:05:26 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
        Build tool maven
        Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
        git revision trunk / b0b535d
        Default Java 1.8.0_144
        findbugs v3.1.0-RC1
        checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/13199/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt
        unit https://builds.apache.org/job/PreCommit-HADOOP-Build/13199/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
        Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/13199/testReport/
        modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
        Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/13199/console
        Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 16s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.       trunk Compile Tests +1 mvninstall 13m 52s trunk passed +1 compile 14m 10s trunk passed +1 checkstyle 0m 36s trunk passed +1 mvnsite 1m 0s trunk passed +1 findbugs 1m 23s trunk passed +1 javadoc 0m 51s trunk passed       Patch Compile Tests +1 mvninstall 0m 38s the patch passed +1 compile 11m 53s the patch passed +1 javac 11m 53s the patch passed -0 checkstyle 0m 40s hadoop-common-project/hadoop-common: The patch generated 9 new + 10 unchanged - 0 fixed = 19 total (was 10) +1 mvnsite 1m 0s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 36s the patch passed +1 javadoc 0m 50s the patch passed       Other Tests -1 unit 8m 46s hadoop-common in the patch failed. +1 asflicense 0m 30s The patch does not generate ASF License warnings. 59m 55s Reason Tests Failed junit tests hadoop.fs.sftp.TestSFTPFileSystem   hadoop.security.TestKDiag Subsystem Report/Notes Docker Image:yetus/hadoop:71bbb86 JIRA Issue HADOOP-14843 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12885930/HADOOP-14843.02.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 8cbbe3c25c1a 3.13.0-117-generic #164-Ubuntu SMP Fri Apr 7 11:05:26 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / b0b535d Default Java 1.8.0_144 findbugs v3.1.0-RC1 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/13199/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt unit https://builds.apache.org/job/PreCommit-HADOOP-Build/13199/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/13199/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/13199/console Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        bharatviswa Bharat Viswanadham added a comment - - edited

        Jason Lowe Agreed, the test code will be reduced. Updated the code. Please review the changes.

        Show
        bharatviswa Bharat Viswanadham added a comment - - edited Jason Lowe Agreed, the test code will be reduced. Updated the code. Please review the changes.
        Hide
        jlowe Jason Lowe added a comment -

        Sorry, I misread the code. I thought it was only testing the bits (i.e.: only checking the READ bit when asking for "+r"), but I see that it is checking all the user permission bits when checking if just the READ bit was set.

        However I think my point is still valid for these tests wrt. the sticky bit. For example, if FsPermission had a bug where FsPermission("+r") acted like mode 1444 instead of 0444 then this test in the patch would pass because it's not checking all of the permission bits:

            FsPermission fs2 = new FsPermission("+r");
            assertEquals(READ, fs2.getUserAction());
            assertEquals(READ, fs2.getGroupAction());
            assertEquals(READ, fs2.getOtherAction());
        

        That's why I'd rather see the test just check against the integer value of the permission, so it is testing all of the bits for every test case. It also ends up being a lot less test code.

        Show
        jlowe Jason Lowe added a comment - Sorry, I misread the code. I thought it was only testing the bits (i.e.: only checking the READ bit when asking for "+r"), but I see that it is checking all the user permission bits when checking if just the READ bit was set. However I think my point is still valid for these tests wrt. the sticky bit. For example, if FsPermission had a bug where FsPermission("+r") acted like mode 1444 instead of 0444 then this test in the patch would pass because it's not checking all of the permission bits: FsPermission fs2 = new FsPermission( "+r" ); assertEquals(READ, fs2.getUserAction()); assertEquals(READ, fs2.getGroupAction()); assertEquals(READ, fs2.getOtherAction()); That's why I'd rather see the test just check against the integer value of the permission, so it is testing all of the bits for every test case. It also ends up being a lot less test code.
        Hide
        bharatviswa Bharat Viswanadham added a comment -

        Jason Lowe Thanks for comments. One point I have not understood from your comment how can FsPermission("+r") could act like mode 0777 and still pass the tests as they are written in this patch. What do you mean by this.

        As +r is 0444 right?

        Show
        bharatviswa Bharat Viswanadham added a comment - Jason Lowe Thanks for comments. One point I have not understood from your comment how can FsPermission("+r") could act like mode 0777 and still pass the tests as they are written in this patch. What do you mean by this. As +r is 0444 right?
        Hide
        jlowe Jason Lowe added a comment -

        Thanks for the patch! I'd rather see checks against the full mode bits, like:

          assertEquals(0444, new FsPermission("+r").toShort());
        

        rather than the explicit perm bit checking. The problem with the latter is that FsPermission("+r") could act like mode 0777 and still pass the tests as they are written in this patch. Doing a check against the integer value means we're also testing the bits that should not be set.

        Show
        jlowe Jason Lowe added a comment - Thanks for the patch! I'd rather see checks against the full mode bits, like: assertEquals(0444, new FsPermission( "+r" ).toShort()); rather than the explicit perm bit checking. The problem with the latter is that FsPermission("+r") could act like mode 0777 and still pass the tests as they are written in this patch. Doing a check against the integer value means we're also testing the bits that should not be set.
        Hide
        bharatviswa Bharat Viswanadham added a comment - - edited

        Andrew Wang Jason Lowe Arpit AgarwalAttached the patch with UT's. Could you please review the patch.

        Show
        bharatviswa Bharat Viswanadham added a comment - - edited Andrew Wang Jason Lowe Arpit Agarwal Attached the patch with UT's. Could you please review the patch.
        Hide
        bharatviswa Bharat Viswanadham added a comment -

        Andrew Wang I think adding test cases for FsPermission(String mode), would be a nice one to have.
        Will upload a patch for this.

        Show
        bharatviswa Bharat Viswanadham added a comment - Andrew Wang I think adding test cases for FsPermission(String mode), would be a nice one to have. Will upload a patch for this.
        Hide
        bharatviswa Bharat Viswanadham added a comment - - edited

        Jason Lowe
        I have thought +r, +rwx are also not valid inputs and provided the patch to raise exception for those inputs also. And as valueOf(String) method is already there for unix symbolic representation for considering these.
        Thanks for clarification.
        I think then existing behavior is fine.

        Show
        bharatviswa Bharat Viswanadham added a comment - - edited Jason Lowe I have thought +r, +rwx are also not valid inputs and provided the patch to raise exception for those inputs also. And as valueOf(String) method is already there for unix symbolic representation for considering these. Thanks for clarification. I think then existing behavior is fine.
        Hide
        andrew.wang Andrew Wang added a comment -

        Should we add unit testing for these cases then, as documentation?

        Show
        andrew.wang Andrew Wang added a comment - Should we add unit testing for these cases then, as documentation?
        Hide
        jlowe Jason Lowe added a comment -

        Thanks for the patch!

        I believe the proposed change will break backwards compatibility. For example, "+r" is a valid input that should return mode 0444 and "+rwx" should return mode 0777. The patch makes those invalid.

        After thinking about this more, I'm leaning towards closing this JIRA as Invalid. "-rwr" unfortunately appears to be a valid symbolic argument even for POSIX chmod. It means "remove read and write for all" because "-rwr" is semantically equivalent to "-rw" because it ignores duplicate flags. The only thing I can think of to catch this case of misunderstanding by the user would be to treat duplicate flags as errors, but since chmod allows them I'm reluctant to diverge from that behavior.

        Show
        jlowe Jason Lowe added a comment - Thanks for the patch! I believe the proposed change will break backwards compatibility. For example, "+r" is a valid input that should return mode 0444 and "+rwx" should return mode 0777. The patch makes those invalid. After thinking about this more, I'm leaning towards closing this JIRA as Invalid. "-rwr" unfortunately appears to be a valid symbolic argument even for POSIX chmod . It means "remove read and write for all" because "-rwr" is semantically equivalent to "-rw" because it ignores duplicate flags. The only thing I can think of to catch this case of misunderstanding by the user would be to treat duplicate flags as errors, but since chmod allows them I'm reluctant to diverge from that behavior.
        Hide
        jlowe Jason Lowe added a comment -

        I believe HADOOP-13508 triggered the behavior change from interpreting the malformed argument as mode 0000 instead of mode 0777. However I wouldn't say this bug was caused by that JIRA. The FsPermission string constructor was accepting this malformed argument both before and after HADOOP-13508, and it should have thrown an error in either case.

        Show
        jlowe Jason Lowe added a comment - I believe HADOOP-13508 triggered the behavior change from interpreting the malformed argument as mode 0000 instead of mode 0777. However I wouldn't say this bug was caused by that JIRA. The FsPermission string constructor was accepting this malformed argument both before and after HADOOP-13508 , and it should have thrown an error in either case.
        Hide
        jojochuang Wei-Chiu Chuang added a comment -

        Is it caused by HADOOP-13508? HADOOP-13508 is/will be in 2.8.2 though

        Show
        jojochuang Wei-Chiu Chuang added a comment - Is it caused by HADOOP-13508 ? HADOOP-13508 is/will be in 2.8.2 though

          People

          • Assignee:
            bharatviswa Bharat Viswanadham
            Reporter:
            jlowe Jason Lowe
          • Votes:
            0 Vote for this issue
            Watchers:
            10 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development