HADOOP-7249. Refactor the chmod/chown/chgrp command to conform to new FsCommand class. Contributed by Daryn Sharp

git-svn-id: https://svn.apache.org/repos/asf/hadoop/common/trunk@1100356 13f79535-47bb-0310-9956-ffa450edef68

CHANGES.txt

@@ -118,6 +118,9 @@ Trunk (unreleased changes)
     HADOOP-7250. Refactor the setrep command to conform to new FsCommand class.
     (Daryn Sharp via szetszwo)
 
+    HADOOP-7249. Refactor the chmod/chown/chgrp command to conform to new
+    FsCommand class.  (Daryn Sharp via szetszwo)
+
   OPTIMIZATIONS
 
   BUG FIXES

src/java/org/apache/hadoop/fs/FsShell.java

@@ -969,114 +969,6 @@ public class FsShell extends Configured implements Tool {
     return getTrash().getCurrentTrashDir();
   }
 
-  /**
-   * This class runs a command on a given FileStatus. This can be used for
-   * running various commands like chmod, chown etc.
-   */
-  static abstract class CmdHandler {
-    
-    protected int errorCode = 0;
-    protected boolean okToContinue = true;
-    protected String cmdName;
-    
-    int getErrorCode() { return errorCode; }
-    boolean okToContinue() { return okToContinue; }
-    String getName() { return cmdName; }
-    
-    protected CmdHandler(String cmdName) {
-      this.cmdName = cmdName;
-    }
-    
-    public abstract void run(FileStatus file, FileSystem fs) throws IOException;
-  }
-  
-  /** helper returns listStatus() */
-  private static FileStatus[] shellListStatus(String cmd, 
-                                              FileSystem srcFs,
-                                              FileStatus src) {
-    if (src.isFile()) {
-      FileStatus[] files = { src };
-      return files;
-    }
-    Path path = src.getPath();
-    try {
-      FileStatus[] files = srcFs.listStatus(path);
-
-      return files;
-    } catch(FileNotFoundException fnfe) {
-      System.err.println(cmd + ": could not get listing for '" + path + "'");
-    } catch (IOException e) {
-      LOG.debug("Error listing " + path, e);
-      System.err.println(cmd + 
-                         ": could not get get listing for '" + path + "' : " +
-                         e.getMessage().split("\n")[0]);
-    }
-    return null;
-  }
-  
-  
-  /**
-   * Runs the command on a given file with the command handler. 
-   * If recursive is set, command is run recursively.
-   */                                       
-  private static int runCmdHandler(CmdHandler handler, FileStatus stat, 
-                                   FileSystem srcFs, 
-                                   boolean recursive) throws IOException {
-    int errors = 0;
-    handler.run(stat, srcFs);
-    if (recursive && stat.isDirectory() && handler.okToContinue()) {
-      FileStatus[] files = shellListStatus(handler.getName(), srcFs, stat);
-      if (files == null) {
-        return 1;
-      }
-      for(FileStatus file : files ) {
-        errors += runCmdHandler(handler, file, srcFs, recursive);
-      }
-    }
-    return errors;
-  }
-
-  ///top level runCmdHandler
-  int runCmdHandler(CmdHandler handler, String[] args,
-                                   int startIndex, boolean recursive) 
-                                   throws IOException {
-    int errors = 0;
-    
-    for (int i=startIndex; i<args.length; i++) {
-      Path srcPath = new Path(args[i]);
-      FileSystem srcFs = srcPath.getFileSystem(getConf());
-      Path[] paths = FileUtil.stat2Paths(srcFs.globStatus(srcPath), srcPath);
-      // if nothing matches to given glob pattern then increment error count
-      if(paths.length==0) {
-        System.err.println(handler.getName() + 
-            ": could not get status for '" + args[i] + "'");
-        errors++;
-      }
-      for(Path path : paths) {
-        try {
-          FileStatus file = srcFs.getFileStatus(path);
-          if (file == null) {
-            System.err.println(handler.getName() + 
-                               ": could not get status for '" + path + "'");
-            errors++;
-          } else {
-            errors += runCmdHandler(handler, file, srcFs, recursive);
-          }
-        } catch (IOException e) {
-          LOG.debug("Error getting status for " + path, e);
-          String msg = (e.getMessage() != null ? e.getLocalizedMessage() :
-            (e.getCause().getMessage() != null ? 
-                e.getCause().getLocalizedMessage() : "null"));
-          System.err.println(handler.getName() + ": could not get status for '"
-                                        + path + "': " + msg.split("\n")[0]);
-          errors++;
-        }
-      }
-    }
-    
-    return (errors > 0 || handler.getErrorCode() != 0) ? 1 : 0;
-  }
-  
   /**
    * Return an abbreviated English-language desc of the byte length
    * @deprecated Consider using {@link org.apache.hadoop.util.StringUtils#byteDesc} instead.
@@ -1107,10 +999,7 @@ public class FsShell extends Configured implements Tool {
       "[" + COPYTOLOCAL_SHORT_USAGE + "] [-moveToLocal <src> <localdst>]\n\t" +
       "[-report]\n\t" +
       "[-touchz <path>] [-test -[ezd] <path>] [-stat [format] <path>]\n\t" +
-      "[-text <path>]\n\t" +
+      "[-text <path>]";
-      "[" + FsShellPermissions.CHMOD_USAGE + "]\n\t" +
-      "[" + FsShellPermissions.CHOWN_USAGE + "]\n\t" +
-      "[" + FsShellPermissions.CHGRP_USAGE + "]";
 
     String conf ="-conf <configuration file>:  Specify an application configuration file.";
  
@@ -1206,38 +1095,6 @@ public class FsShell extends Configured implements Tool {
       "\t\tin the specified format. Format accepts filesize in blocks (%b), filename (%n),\n" +
       "\t\tblock size (%o), replication (%r), modification date (%y, %Y)\n";
     
-    String chmod = FsShellPermissions.CHMOD_USAGE + "\n" +
-      "\t\tChanges permissions of a file.\n" +
-      "\t\tThis works similar to shell's chmod with a few exceptions.\n\n" +
-      "\t-R\tmodifies the files recursively. This is the only option\n" +
-      "\t\tcurrently supported.\n\n" +
-      "\tMODE\tMode is same as mode used for chmod shell command.\n" +
-      "\t\tOnly letters recognized are 'rwxXt'. E.g. +t,a+r,g-w,+rwx,o=r\n\n" +
-      "\tOCTALMODE Mode specifed in 3 or 4 digits. If 4 digits, the first may\n" +
-      "\tbe 1 or 0 to turn the sticky bit on or off, respectively.  Unlike " +
-      "\tshell command, it is not possible to specify only part of the mode\n" +
-      "\t\tE.g. 754 is same as u=rwx,g=rx,o=r\n\n" +
-      "\t\tIf none of 'augo' is specified, 'a' is assumed and unlike\n" +
-      "\t\tshell command, no umask is applied.\n";
-    
-    String chown = FsShellPermissions.CHOWN_USAGE + "\n" +
-      "\t\tChanges owner and group of a file.\n" +
-      "\t\tThis is similar to shell's chown with a few exceptions.\n\n" +
-      "\t-R\tmodifies the files recursively. This is the only option\n" +
-      "\t\tcurrently supported.\n\n" +
-      "\t\tIf only owner or group is specified then only owner or\n" +
-      "\t\tgroup is modified.\n\n" +
-      "\t\tThe owner and group names may only cosists of digits, alphabet,\n"+
-      "\t\tand any of '-_.@/' i.e. [-_.@/a-zA-Z0-9]. The names are case\n" +
-      "\t\tsensitive.\n\n" +
-      "\t\tWARNING: Avoid using '.' to separate user name and group though\n" +
-      "\t\tLinux allows it. If user names have dots in them and you are\n" +
-      "\t\tusing local file system, you might see surprising results since\n" +
-      "\t\tshell command 'chown' is used for local files.\n";
-    
-    String chgrp = FsShellPermissions.CHGRP_USAGE + "\n" +
-      "\t\tThis is equivalent to -chown ... :GROUP ...\n";
-
     String expunge = "-expunge: Empty the Trash.\n";
     
     String help = "-help [cmd]: \tDisplays help for given command or all commands if none\n" +
@@ -1294,12 +1151,6 @@ public class FsShell extends Configured implements Tool {
       System.out.println(text);
     } else if ("stat".equals(cmd)) {
       System.out.println(stat);
-    } else if ("chmod".equals(cmd)) {
-      System.out.println(chmod);
-    } else if ("chown".equals(cmd)) {
-      System.out.println(chown);
-    } else if ("chgrp".equals(cmd)) {
-      System.out.println(chgrp);
     } else if ("help".equals(cmd)) {
       System.out.println(help);
     } else {
@@ -1330,9 +1181,6 @@ public class FsShell extends Configured implements Tool {
       System.out.println(test);
       System.out.println(text);
       System.out.println(stat);
-      System.out.println(chmod);
-      System.out.println(chown);      
-      System.out.println(chgrp);
 
       for (String thisCmdName : commandFactory.getNames()) {
         printHelp(commandFactory.getInstance(thisCmdName));
@@ -1503,9 +1351,6 @@ public class FsShell extends Configured implements Tool {
       System.err.println("           [-touchz <path>]");
       System.err.println("           [-test -[ezd] <path>]");
       System.err.println("           [-stat [format] <path>]");
-      System.err.println("           [" + FsShellPermissions.CHMOD_USAGE + "]");      
-      System.err.println("           [" + FsShellPermissions.CHOWN_USAGE + "]");
-      System.err.println("           [" + FsShellPermissions.CHGRP_USAGE + "]");
       for (String name : commandFactory.getNames()) {
       	instance = commandFactory.getInstance(name);
         System.err.println("           [" + instance.getUsage() + "]");
@@ -1615,10 +1460,6 @@ public class FsShell extends Configured implements Tool {
         exitCode = doall(cmd, argv, i);
       } else if ("-moveToLocal".equals(cmd)) {
         moveToLocal(argv[i++], new Path(argv[i++]));
-      } else if ("-chmod".equals(cmd) || 
-                 "-chown".equals(cmd) ||
-                 "-chgrp".equals(cmd)) {
-        exitCode = FsShellPermissions.changePermissions(cmd, argv, i, this);
       } else if ("-mv".equals(cmd)) {
         exitCode = rename(argv, getConf());
       } else if ("-cp".equals(cmd)) {

src/java/org/apache/hadoop/fs/FsShellPermissions.java

@@ -18,15 +18,19 @@
 package org.apache.hadoop.fs;
 
 import java.io.IOException;
+import java.util.LinkedList;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
 import org.apache.commons.logging.Log;
 import org.apache.hadoop.classification.InterfaceAudience;
 import org.apache.hadoop.classification.InterfaceStability;
-import org.apache.hadoop.fs.FsShell.CmdHandler;
-import org.apache.hadoop.fs.permission.FsPermission;
 import org.apache.hadoop.fs.permission.ChmodParser;
+import org.apache.hadoop.fs.permission.FsPermission;
+import org.apache.hadoop.fs.shell.CommandFactory;
+import org.apache.hadoop.fs.shell.CommandFormat;
+import org.apache.hadoop.fs.shell.FsCommand;
+import org.apache.hadoop.fs.shell.PathData;
 
 
 /**
@@ -35,80 +39,131 @@ import org.apache.hadoop.fs.permission.ChmodParser;
  */
 @InterfaceAudience.Private
 @InterfaceStability.Unstable
-class FsShellPermissions {
+public class FsShellPermissions extends FsCommand {
 
   static Log LOG = FsShell.LOG;
   
-  /*========== chmod ==========*/
+  /**
+   * Register the permission related commands with the factory
+   * @param factory the command factory
+   */
+  public static void registerCommands(CommandFactory factory) {
+    factory.addClass(Chmod.class, "-chmod");
+    factory.addClass(Chown.class, "-chown");
+    factory.addClass(Chgrp.class, "-chgrp");
+  }
 
-  /*
+  @Override
+  protected String getFnfText(Path path) {
+    // TODO: printing the path twice is silly for backwards compatibility
+    return "could not get status for '"+path+"': File does not exist: "+path;   
+  }
+
+  /**
    * The pattern is almost as flexible as mode allowed by chmod shell command.
    * The main restriction is that we recognize only rwxXt. To reduce errors we
    * also enforce octal mode specifications of either 3 digits without a sticky
    * bit setting or four digits with a sticky bit setting.
    */
+  public static class Chmod extends FsShellPermissions {
+    public static final String NAME = "chmod";
+    public static final String USAGE = "[-R] <MODE[,MODE]... | OCTALMODE> PATH...";
+    public static final String DESCRIPTION =
+      "Changes permissions of a file.\n" +
+      "\tThis works similar to shell's chmod with a few exceptions.\n\n" +
+      "-R\tmodifies the files recursively. This is the only option\n" +
+      "\tcurrently supported.\n\n" +
+      "MODE\tMode is same as mode used for chmod shell command.\n" +
+      "\tOnly letters recognized are 'rwxXt'. E.g. +t,a+r,g-w,+rwx,o=r\n\n" +
+      "OCTALMODE Mode specifed in 3 or 4 digits. If 4 digits, the first may\n" +
+      "be 1 or 0 to turn the sticky bit on or off, respectively.  Unlike " +
+      "shell command, it is not possible to specify only part of the mode\n" +
+      "\tE.g. 754 is same as u=rwx,g=rx,o=r\n\n" +
+      "\tIf none of 'augo' is specified, 'a' is assumed and unlike\n" +
+      "\tshell command, no umask is applied.";
 
-  static String CHMOD_USAGE =
+    protected ChmodParser pp;
-                            "-chmod [-R] <MODE[,MODE]... | OCTALMODE> PATH...";
 
-  private static  ChmodParser pp;
+    @Override
+    protected void processOptions(LinkedList<String> args) throws IOException {
+      CommandFormat cf = new CommandFormat(null, 2, Integer.MAX_VALUE, "R", null);
+      cf.parse(args);
+      setRecursive(cf.getOpt("R"));
 
-  private static class ChmodHandler extends CmdHandler {
+      String modeStr = args.removeFirst();
-
-    ChmodHandler(String modeStr) throws IOException {
-      super("chmod");
       try {
         pp = new ChmodParser(modeStr);
       } catch (IllegalArgumentException iea) {
-        patternError(iea.getMessage());
+        // TODO: remove "chmod : " so it's not doubled up in output, but it's
+        // here for backwards compatibility...
+        throw new IllegalArgumentException(
+            "chmod : mode '" + modeStr + "' does not match the expected pattern.");      
       }
     }
     
-    private void patternError(String mode) throws IOException {
-     throw new IOException("chmod : mode '" + mode + 
-         "' does not match the expected pattern.");      
-    }
-    
     @Override
-    public void run(FileStatus file, FileSystem srcFs) throws IOException {
+    protected void processPath(PathData item) throws IOException {
-      int newperms = pp.applyNewPermission(file);
+      short newperms = pp.applyNewPermission(item.stat);
-
+      if (item.stat.getPermission().toShort() != newperms) {
-      if (file.getPermission().toShort() != newperms) {
         try {
-          srcFs.setPermission(file.getPath(), 
+          item.fs.setPermission(item.path, new FsPermission(newperms));
-                                new FsPermission((short)newperms));
         } catch (IOException e) {
-          LOG.debug("Error changing permissions of " + file.getPath(), e);
+          LOG.debug("Error changing permissions of " + item, e);
-          System.err.println(getName() + ": changing permissions of '" + 
+          throw new IOException(
-                             file.getPath() + "':" + e.getMessage());
+              "changing permissions of '" + item + "': " + e.getMessage());
         }
       }
     }    
   }
   
-  /*========== chown ==========*/
+  // used by chown/chgrp
-  
   static private String allowedChars = "[-_./@a-zA-Z0-9]";  
+
+  /**
+   * Used to change owner and/or group of files 
+   */
+  public static class Chown extends FsShellPermissions {
+    public static final String NAME = "chown";
+    public static final String USAGE = "[-R] [OWNER][:[GROUP]] PATH...";
+    public static final String DESCRIPTION =
+      "Changes owner and group of a file.\n" +
+      "\tThis is similar to shell's chown with a few exceptions.\n\n" +
+      "\t-R\tmodifies the files recursively. This is the only option\n" +
+      "\tcurrently supported.\n\n" +
+      "\tIf only owner or group is specified then only owner or\n" +
+      "\tgroup is modified.\n\n" +
+      "\tThe owner and group names may only cosists of digits, alphabet,\n"+
+      "\tand any of '-_.@/' i.e. [-_.@/a-zA-Z0-9]. The names are case\n" +
+      "\tsensitive.\n\n" +
+      "\tWARNING: Avoid using '.' to separate user name and group though\n" +
+      "\tLinux allows it. If user names have dots in them and you are\n" +
+      "\tusing local file system, you might see surprising results since\n" +
+      "\tshell command 'chown' is used for local files.";
+
     ///allows only "allowedChars" above in names for owner and group
-  static private Pattern chownPattern = 
+    static private final Pattern chownPattern = Pattern.compile(
-         Pattern.compile("^\\s*(" + allowedChars + "+)?" +
+        "^\\s*(" + allowedChars + "+)?([:](" + allowedChars + "*))?\\s*$");
-                          "([:](" + allowedChars + "*))?\\s*$");
-  static private Pattern chgrpPattern = 
-         Pattern.compile("^\\s*(" + allowedChars + "+)\\s*$");
 
-  static String CHOWN_USAGE = "-chown [-R] [OWNER][:[GROUP]] PATH...";
-  static String CHGRP_USAGE = "-chgrp [-R] GROUP PATH...";  
-
-  private static class ChownHandler extends CmdHandler {
     protected String owner = null;
     protected String group = null;
 
-    ChownHandler(String ownerStr) throws IOException {
+    @Override
-      super("chown");
+    protected void processOptions(LinkedList<String> args) throws IOException {
+      CommandFormat cf = new CommandFormat(null, 2, Integer.MAX_VALUE, "R");
+      cf.parse(args);
+      setRecursive(cf.getOpt("R"));
+      parseOwnerGroup(args.removeFirst());
+    }
+    
+    /**
+     * Parse the first argument into an owner and group
+     * @param ownerStr string describing new ownership
+     */
+    protected void parseOwnerGroup(String ownerStr) {
       Matcher matcher = chownPattern.matcher(ownerStr);
       if (!matcher.matches()) {
-        throw new IOException("'" + ownerStr + "' does not match " +
+        throw new IllegalArgumentException(
-                              "expected pattern for [owner][:group].");
+            "'" + ownerStr + "' does not match expected pattern for [owner][:group].");
       }
       owner = matcher.group(1);
       group = matcher.group(3);
@@ -116,91 +171,52 @@ class FsShellPermissions {
         group = null;
       }
       if (owner == null && group == null) {
-        throw new IOException("'" + ownerStr + "' does not specify " +
+        throw new IllegalArgumentException(
-                              " onwer or group.");
+            "'" + ownerStr + "' does not specify owner or group.");
       }    
     }
     
     @Override
-    public void run(FileStatus file, FileSystem srcFs) throws IOException {
+    protected void processPath(PathData item) throws IOException {
       //Should we do case insensitive match?
-      String newOwner = (owner == null || owner.equals(file.getOwner())) ?
+      String newOwner = (owner == null || owner.equals(item.stat.getOwner())) ?
                         null : owner;
-      String newGroup = (group == null || group.equals(file.getGroup())) ?
+      String newGroup = (group == null || group.equals(item.stat.getGroup())) ?
                         null : group;
 
       if (newOwner != null || newGroup != null) {
         try {
-          srcFs.setOwner(file.getPath(), newOwner, newGroup);
+          item.fs.setOwner(item.path, newOwner, newGroup);
         } catch (IOException e) {
-          LOG.debug("Error changing ownership of " + file.getPath(), e);
+          LOG.debug("Error changing ownership of " + item, e);
-          System.err.println(getName() + ": changing ownership of '" + 
+          throw new IOException(
-                             file.getPath() + "':" + e.getMessage());
+              "changing ownership of '" + item + "': " + e.getMessage());
-
         }
       }
     }
   }
 
-  /*========== chgrp ==========*/    
+  /**
+   * Used to change group of files 
+   */
+  public static class Chgrp extends Chown {
+    public static final String NAME = "chgrp";
+    public static final String USAGE = "[-R] GROUP PATH...";
+    public static final String DESCRIPTION =
+      "This is equivalent to -chown ... :GROUP ...";
 
-  private static class ChgrpHandler extends CmdHandler {
+    static private final Pattern chgrpPattern = 
-    protected String group = null;
+      Pattern.compile("^\\s*(" + allowedChars + "+)\\s*$");
-    ChgrpHandler(String groupStr) throws IOException {
-      super("chgrp");
-
-      Matcher matcher = chgrpPattern.matcher(groupStr);
-      if (!matcher.matches()) {
-        throw new IOException("'" + groupStr + "' does not match " +
-        "expected pattern for group");
-      }
-      group = matcher.group(1);
-    }
 
     @Override
-    public void run(FileStatus file, FileSystem srcFs) throws IOException {
+    protected void parseOwnerGroup(String groupStr) {
-
+      Matcher matcher = chgrpPattern.matcher(groupStr);
-      String newGroup = (group.equals(file.getGroup())) ?
+      if (!matcher.matches()) {
-                        null : group;
+        throw new IllegalArgumentException(
-
+            "'" + groupStr + "' does not match expected pattern for group");
-      if (newGroup != null) {
+      }
-        try {
+      owner = null;
-          srcFs.setOwner(file.getPath(), null, newGroup);
+      group = matcher.group(1);
-        } catch (IOException e) {
-          LOG.debug("Error changing ownership of " + file.getPath(), e);
-          System.err.println(getName() + ": changing ownership of '" + 
-                             file.getPath() + "':" + e.getMessage());
-
     }
   }
 }
-    
-  }
-
-  static int changePermissions(String cmd, 
-                                String argv[], int startIndex, FsShell shell)
-                                throws IOException {
-    CmdHandler handler = null;
-    boolean recursive = false;
-
-    // handle common arguments, currently only "-R" 
-    for (; startIndex < argv.length && argv[startIndex].equals("-R"); 
-    startIndex++) {
-      recursive = true;
-    }
-
-    if ( startIndex >= argv.length ) {
-      throw new IOException("Not enough arguments for the command");
-    }
-
-    if (cmd.equals("-chmod")) {
-      handler = new ChmodHandler(argv[startIndex++]);
-    } else if (cmd.equals("-chown")) {
-      handler = new ChownHandler(argv[startIndex++]);
-    } else if (cmd.equals("-chgrp")) {
-      handler = new ChgrpHandler(argv[startIndex++]);
-    }
-
-    return shell.runCmdHandler(handler, argv, startIndex, recursive);
-  } 
-}

src/java/org/apache/hadoop/fs/shell/Command.java

@@ -144,10 +144,17 @@ abstract public class Command extends Configured {
       displayError(e);
     }
     
-    // TODO: -1 should be reserved for syntax error, 1 should be failure
+    return (numErrors == 0) ? exitCode : exitCodeForError();
-    return (numErrors == 0) ? exitCode : -1;
   }
 
+  /**
+   * The exit code to be returned if any errors occur during execution.
+   * This method is needed to account for the inconsistency in the exit
+   * codes returned by various commands.
+   * @return a non-zero exit code
+   */
+  protected int exitCodeForError() { return 1; }
+  
   /**
    * Must be implemented by commands to process the command line flags and
    * check the bounds of the remaining arguments.  If an

src/java/org/apache/hadoop/fs/shell/CommandFormat.java

@@ -32,15 +32,21 @@ public class CommandFormat {
   final String name;
   final int minPar, maxPar;
   final Map<String, Boolean> options = new HashMap<String, Boolean>();
+  boolean ignoreUnknownOpts = false;
   
   /** constructor */
   public CommandFormat(String n, int min, int max, String ... possibleOpt) {
     name = n;
     minPar = min;
     maxPar = max;
-    for(String opt : possibleOpt)
+    for (String opt : possibleOpt) {
+      if (opt == null) {
+        ignoreUnknownOpts = true;
+      } else {
         options.put(opt, Boolean.FALSE);
       }
+    }
+  }
 
   /** Parse parameters starting from the given position
    * Consider using the variant that directly takes a List
@@ -67,14 +73,14 @@ public class CommandFormat {
       String arg = args.get(pos);
       if (arg.startsWith("-") && arg.length() > 1) {
         String opt = arg.substring(1);
-        if (!options.containsKey(opt)) {
+        if (options.containsKey(opt)) {
-          throw new UnknownOptionException(arg);
-        }
           args.remove(pos);
           options.put(opt, Boolean.TRUE);
-      } else {
+          continue;
-        pos++;
         }
+        if (!ignoreUnknownOpts) throw new UnknownOptionException(arg);
+      }
+      pos++;
     }
     int psize = args.size();
     if (psize < minPar) {

src/java/org/apache/hadoop/fs/shell/FsCommand.java

@@ -23,6 +23,7 @@ import java.io.IOException;
 import org.apache.hadoop.classification.InterfaceAudience;
 import org.apache.hadoop.classification.InterfaceStability;
 import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.FsShellPermissions;
 import org.apache.hadoop.fs.Path;
 
 /**
@@ -43,6 +44,7 @@ abstract public class FsCommand extends Command {
    */
   public static void registerCommands(CommandFactory factory) {
     factory.registerCommands(Count.class);
+    factory.registerCommands(FsShellPermissions.class);
     factory.registerCommands(Ls.class);
     factory.registerCommands(Mkdir.class);
     factory.registerCommands(SetReplication.class);

src/java/org/apache/hadoop/fs/shell/Ls.java

@@ -132,6 +132,9 @@ class Ls extends FsCommand {
     return "Cannot access " + path.toUri() + ": No such file or directory.";
   }
 
+  @Override
+  protected int exitCodeForError() { return -1; }
+
   /**
    * Get a recursive listing of all files in that match the file patterns.
    * Same as "-ls -R"

src/test/core/org/apache/hadoop/cli/testConf.xml

@@ -625,11 +625,7 @@
       <comparators>
         <comparator>
           <type>RegexpComparator</type>
-          <expected-output>^-chmod \[-R\] &lt;MODE\[,MODE\]... \| OCTALMODE&gt; PATH...( )*</expected-output>
+          <expected-output>^-chmod \[-R\] &lt;MODE\[,MODE\]... \| OCTALMODE&gt; PATH...:( |\t)*Changes permissions of a file.( )*</expected-output>
-        </comparator>
-        <comparator>
-          <type>RegexpComparator</type>
-          <expected-output>^( |\t)*Changes permissions of a file.( )*</expected-output>
         </comparator>
         <comparator>
           <type>RegexpComparator</type>
@@ -684,11 +680,7 @@
       <comparators>
         <comparator>
           <type>RegexpComparator</type>
-          <expected-output>^-chown \[-R\] \[OWNER\]\[:\[GROUP\]\] PATH...( )*</expected-output>
+          <expected-output>^-chown \[-R\] \[OWNER\]\[:\[GROUP\]\] PATH...:( |\t)*Changes owner and group of a file.( )*</expected-output>
-        </comparator>
-        <comparator>
-          <type>RegexpComparator</type>
-          <expected-output>^( |\t)*Changes owner and group of a file.( )*</expected-output>
         </comparator>
         <comparator>
           <type>RegexpComparator</type>
@@ -751,11 +743,7 @@
       <comparators>
         <comparator>
           <type>RegexpComparator</type>
-          <expected-output>^-chgrp \[-R\] GROUP PATH...( )*</expected-output>
+          <expected-output>^-chgrp \[-R\] GROUP PATH...:( |\t)*This is equivalent to -chown ... :GROUP ...( )*</expected-output>
-        </comparator>
-        <comparator>
-          <type>RegexpComparator</type>
-          <expected-output>^( |\t)*This is equivalent to -chown ... :GROUP ...( )*</expected-output>
         </comparator>
       </comparators>
     </test>

src/test/core/org/apache/hadoop/fs/TestFsShellReturnCode.java

@@ -18,18 +18,23 @@
 
 package org.apache.hadoop.fs;
 
+import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertTrue;
 
 import java.io.ByteArrayOutputStream;
 import java.io.IOException;
 import java.io.PrintStream;
-import java.net.URI;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.LinkedList;
+import java.util.List;
 
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
+import org.apache.ftpserver.command.impl.STAT;
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.io.IOUtils;
-import org.junit.Assert;
+import org.junit.BeforeClass;
 import org.junit.Test;
 
 /**
@@ -41,6 +46,15 @@ public class TestFsShellReturnCode {
       .getLog("org.apache.hadoop.fs.TestFsShellReturnCode");
 
   private static final Configuration conf = new Configuration();
+  private static FileSystem fileSys;
+  private static FsShell fsShell;
+  
+  @BeforeClass
+  public static void setup() throws IOException {
+    conf.setClass("fs.file.impl", LocalFileSystemExtn.class, RawLocalFileSystem.class);
+    fileSys = FileSystem.get(conf);
+    fsShell = new FsShell(conf);
+  }
   
   private static String TEST_ROOT_DIR = System.getProperty("test.build.data",
       "build/test/data/testCHReturnCode");
@@ -51,11 +65,47 @@ public class TestFsShellReturnCode {
     stm.close();
   }
 
-  public void verify(String cmd, String argv[], int cmdIndex,
+  private void change(int exit, String owner, String group, String...files)
-      FsShell fsShell, int exitCode) throws Exception {
+  throws Exception {
-    int ec;
+    FileStatus[][] oldStats = new FileStatus[files.length][];
-    ec = FsShellPermissions.changePermissions(cmd, argv, cmdIndex, fsShell);
+    for (int i=0; i < files.length; i++) {
-    Assert.assertEquals(ec, exitCode);
+      oldStats[i] = fileSys.globStatus(new Path(files[i]));
+    }
+    
+    List<String>argv = new LinkedList<String>();
+    if (owner != null) {
+      argv.add("-chown");
+
+      String chown = owner;
+      if (group != null) {
+        chown += ":" + group;
+        if (group.isEmpty()) group = null; // avoid testing for it later
+      }
+      argv.add(chown);
+    } else {
+      argv.add("-chgrp");
+      argv.add(group);
+    }
+    
+    Collections.addAll(argv, files);
+    
+    assertEquals(exit, fsShell.run(argv.toArray(new String[0])));
+
+    for (int i=0; i < files.length; i++) {
+      FileStatus[] stats = fileSys.globStatus(new Path(files[i]));
+      if (stats != null) {
+        for (int j=0; j < stats.length; j++) {
+          assertEquals("check owner of " + files[i],
+              ((owner != null) ? "STUB-"+owner : oldStats[i][j].getOwner()),
+              stats[j].getOwner()
+          );
+          assertEquals("check group of " + files[i],
+              ((group != null) ? "STUB-"+group : oldStats[i][j].getGroup()),
+              stats[j].getGroup()
+          );        
+        }
+      }
+    }
   }
 
   /**
@@ -70,8 +120,6 @@ public class TestFsShellReturnCode {
   @Test
   public void testChmod() throws Exception {
 
-    FsShell fsShell = new FsShell(conf);
-
     final String f1 = TEST_ROOT_DIR + "/" + "testChmod/fileExists";
     final String f2 = TEST_ROOT_DIR + "/" + "testChmod/fileDoesNotExist";
     final String f3 = TEST_ROOT_DIR + "/" + "testChmod/nonExistingfiles*";
@@ -84,23 +132,21 @@ public class TestFsShellReturnCode {
 
     final String f7 = TEST_ROOT_DIR + "/" + "testChmod/file*";
 
-    FileSystem fileSys = FileSystem.getLocal(conf);
-
     // create and write test file
     writeFile(fileSys, p1);
     assertTrue(fileSys.exists(p1));
 
     // Test 1: Test 1: exit code for chmod on existing is 0
     String argv[] = { "-chmod", "777", f1 };
-    verify("-chmod", argv, 1, fsShell, 0);
+    assertEquals(0, fsShell.run(argv));
 
     // Test 2: exit code for chmod on non-existing path is 1
     String argv2[] = { "-chmod", "777", f2 };
-    verify("-chmod", argv2, 1, fsShell, 1);
+    assertEquals(1, fsShell.run(argv2));
 
     // Test 3: exit code for chmod on non-existing path with globbed input is 1
     String argv3[] = { "-chmod", "777", f3 };
-    verify("-chmod", argv3, 1, fsShell, 1);
+    assertEquals(1, fsShell.run(argv3));
 
     // create required files
     writeFile(fileSys, p4);
@@ -112,7 +158,7 @@ public class TestFsShellReturnCode {
 
     // Test 4: exit code for chmod on existing path with globbed input is 0
     String argv4[] = { "-chmod", "777", f7 };
-    verify("-chmod", argv4, 1, fsShell, 0);
+    assertEquals(0, fsShell.run(argv4));
 
   }
 
@@ -128,8 +174,6 @@ public class TestFsShellReturnCode {
   @Test
   public void testChown() throws Exception {
 
-    FsShell fsShell = new FsShell(conf);
-
     final String f1 = TEST_ROOT_DIR + "/" + "testChown/fileExists";
     final String f2 = TEST_ROOT_DIR + "/" + "testChown/fileDoesNotExist";
     final String f3 = TEST_ROOT_DIR + "/" + "testChown/nonExistingfiles*";
@@ -142,23 +186,18 @@ public class TestFsShellReturnCode {
 
     final String f7 = TEST_ROOT_DIR + "/" + "testChown/file*";
 
-    FileSystem fileSys = FileSystem.getLocal(conf);
-
     // create and write test file
     writeFile(fileSys, p1);
     assertTrue(fileSys.exists(p1));
 
     // Test 1: exit code for chown on existing file is 0
-    String argv[] = { "-chown", "admin", f1 };
+    change(0, "admin", null, f1);
-    verify("-chown", argv, 1, fsShell, 0);
 
     // Test 2: exit code for chown on non-existing path is 1
-    String argv2[] = { "-chown", "admin", f2 };
+    change(1, "admin", null, f2);
-    verify("-chown", argv2, 1, fsShell, 1);
 
     // Test 3: exit code for chown on non-existing path with globbed input is 1
-    String argv3[] = { "-chown", "admin", f3 };
+    change(1, "admin", null, f3);
-    verify("-chown", argv3, 1, fsShell, 1);
 
     // create required files
     writeFile(fileSys, p4);
@@ -169,22 +208,11 @@ public class TestFsShellReturnCode {
     assertTrue(fileSys.exists(p6));
 
     // Test 4: exit code for chown on existing path with globbed input is 0
-    String argv4[] = { "-chown", "admin", f7 };
+    change(0, "admin", null, f7);
-    verify("-chown", argv4, 1, fsShell, 0);
 
    //Test 5: test for setOwner invocation on FS from command handler.
-    conf.set("fs.testfs.impl","org.apache.hadoop.fs.TestFsShellReturnCode$LocalFileSystemExtn");
+    change(0, "admin", "Test", f1);
-    final String file = "testfs:///testFile";
+    change(0, "admin", "", f1);
-    LocalFileSystemExtn fileSystem = (LocalFileSystemExtn)FileSystem.get(new URI(file), conf);
-    String argv5[] = { "-chown", "admin:Test", file };
-    FsShellPermissions.changePermissions("-chown", argv5, 1, fsShell);
-    assertTrue("Not invoked the setOwner on Fs",fileSystem.groupname.equals("Test"));
-    assertTrue("Not invoked the setOwner on Fs",fileSystem.username.equals("admin"));
-    String argv6[] = { "-chown", "admin:", file };
-    FsShellPermissions.changePermissions("-chown", argv6, 1, fsShell);
-    assertTrue("Not invoked the setOwner on Fs",fileSystem.groupname == null);
-    assertTrue("Not invoked the setOwner on Fs",fileSystem.username.equals("admin"));
-    
   }
 
   /**
@@ -199,8 +227,6 @@ public class TestFsShellReturnCode {
   @Test
   public void testChgrp() throws Exception {
 
-    FsShell fsShell = new FsShell(conf);
-
     final String f1 = TEST_ROOT_DIR + "/" + "testChgrp/fileExists";
     final String f2 = TEST_ROOT_DIR + "/" + "testChgrp/fileDoesNotExist";
     final String f3 = TEST_ROOT_DIR + "/" + "testChgrp/nonExistingfiles*";
@@ -213,23 +239,20 @@ public class TestFsShellReturnCode {
 
     final String f7 = TEST_ROOT_DIR + "/" + "testChgrp/file*";
 
-    FileSystem fileSys = FileSystem.getLocal(conf);
-
     // create and write test file
     writeFile(fileSys, p1);
     assertTrue(fileSys.exists(p1));
 
     // Test 1: exit code for chgrp on existing file is 0
-    String argv[] = { "-chgrp", "admin", f1 };
+    change(0, null, "admin", f1);
-    verify("-chgrp", argv, 1, fsShell, 0);
 
     // Test 2: exit code for chgrp on non existing path is 1
-    String argv2[] = { "-chgrp", "admin", f2 };
+    change(1, null, "admin", f2);
-    verify("-chgrp", argv2, 1, fsShell, 1);
+    change(1, null, "admin", f2, f1); // exit code used to be for last item
 
     // Test 3: exit code for chgrp on non-existing path with globbed input is 1
-    String argv3[] = { "-chgrp", "admin", f3 };
+    change(1, null, "admin", f3);
-    verify("-chgrp", argv3, 1, fsShell, 1);
+    change(1, null, "admin", f3, f1);
 
     // create required files
     writeFile(fileSys, p4);
@@ -240,9 +263,8 @@ public class TestFsShellReturnCode {
     assertTrue(fileSys.exists(p6));
 
     // Test 4: exit code for chgrp on existing path with globbed input is 0
-    String argv4[] = { "-chgrp", "admin", f7 };
+    change(0, null, "admin", f7);
-    verify("-chgrp", argv4, 1, fsShell, 0);
+    change(1, null, "admin", f2, f7);
-
   }
   
   @Test
@@ -257,7 +279,6 @@ public class TestFsShellReturnCode {
     System.setErr(out);
     final String results;
     try {
-      FileSystem fileSys = FileSystem.getLocal(conf);
       String[] args = new String[3];
       args[0] = "-get";
       args[1] = "/invalidPath";
@@ -275,7 +296,7 @@ public class TestFsShellReturnCode {
   }
   
   @Test
-  public void testInvalidDefautlFS() throws Exception {
+  public void testInvalidDefaultFS() throws Exception {
     // if default fs doesn't exist or is invalid, but the path provided in 
     // arguments is valid - fsshell should work
     FsShell shell = new FsShell();
@@ -306,22 +327,45 @@ public class TestFsShellReturnCode {
   }
   
   static class LocalFileSystemExtn extends RawLocalFileSystem {
+    protected static HashMap<String,String> owners = new HashMap<String,String>();
+    protected static HashMap<String,String> groups = new HashMap<String,String>();
 
-    private String username;
+    @Override
-    private String groupname;
+    public FSDataOutputStream create(Path p) throws IOException {
+      //owners.remove(p);
+      //groups.remove(p);
+      return super.create(p);
+    }
 
     @Override
     public void setOwner(Path p, String username, String groupname)
         throws IOException {
-      this.username = username;
+      String f = makeQualified(p).toString();
-      this.groupname = groupname;
+      if (username != null)  {
-
+        owners.put(f, username);
+      }
+      if (groupname != null) {
+        groups.put(f, groupname);
+      }
     }
 
     @Override
-    public FileStatus getFileStatus(Path f) throws IOException {
+    public FileStatus getFileStatus(Path p) throws IOException {
-      return new FileStatus();
+      String f = makeQualified(p).toString();
-    }
+      FileStatus stat = super.getFileStatus(p);
-  }
       
+      stat.getPermission();
+      if (owners.containsKey(f)) {
+        stat.setOwner("STUB-"+owners.get(f));      
+      } else {
+        stat.setOwner("REAL-"+stat.getOwner());
+      }
+      if (groups.containsKey(f)) {
+        stat.setGroup("STUB-"+groups.get(f));      
+      } else {
+        stat.setGroup("REAL-"+stat.getGroup());
+      }
+      return stat;
+    }
+  }
 }