Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756784AbYCNJhd (ORCPT ); Fri, 14 Mar 2008 05:37:33 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1752710AbYCNJhX (ORCPT ); Fri, 14 Mar 2008 05:37:23 -0400 Received: from ppsw-9.csi.cam.ac.uk ([131.111.8.139]:37775 "EHLO ppsw-9.csi.cam.ac.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752937AbYCNJhU (ORCPT ); Fri, 14 Mar 2008 05:37:20 -0400 X-Greylist: delayed 2499 seconds by postgrey-1.27 at vger.kernel.org; Fri, 14 Mar 2008 05:37:19 EDT X-Cam-SpamDetails: Not scanned X-Cam-AntiVirus: No virus found X-Cam-ScannerInfo: http://www.cam.ac.uk/cs/email/scanner/ From: Anton Altaparmakov To: Shen Feng In-Reply-To: <47D7A1C1.60504@cn.fujitsu.com> Subject: Re: [PATCH] NTFS: update parse_options() with filesystem argument parser References: <47D7A1C1.60504@cn.fujitsu.com> Message-Id: <9BBF956E-A17C-49FE-A59C-0603A4C05F88@cam.ac.uk> Content-Type: text/plain; charset=US-ASCII; format=flowed; delsp=yes Content-Transfer-Encoding: 7bit Mime-Version: 1.0 (Apple Message framework v919.2) Date: Fri, 14 Mar 2008 08:55:25 +0000 Cc: ntfs-dev , LKML X-Mailer: Apple Mail (2.919.2) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 13237 Lines: 433 Hi Sheng, Thanks for your patch but I much prefer the original code! For a start you added 60 lines of code, that is hardly a cleanup... And I think the standard file system argument parser is ugly. The NTFS parse is nice and readable by comparison. (-; Best regards, Anton On 12 Mar 2008, at 09:26, Shen Feng wrote: > ntfs driver uses the macros defined by itself to parse the > filesystem argument. this makes the code look ugly. change it by > using the standard filesystem argument parser. > > Signed-off-by: Shen Feng > --- > fs/ntfs/super.c | 340 +++++++++++++++++++++++++++++++ > +----------------------- > 1 files changed, 200 insertions(+), 140 deletions(-) > > diff --git a/fs/ntfs/super.c b/fs/ntfs/super.c > index 3e76f3b..232cb1a 100644 > --- a/fs/ntfs/super.c > +++ b/fs/ntfs/super.c > @@ -31,6 +31,7 @@ > #include > #include > #include > +#include > > #include "sysctl.h" > #include "logfile.h" > @@ -69,24 +70,91 @@ const option_t on_errors_arr[] = { > { 0, NULL } > }; > > -/** > - * simple_getbool - > - * > - * Copied from old ntfs driver (which copied from vfat driver). > - */ > -static int simple_getbool(char *s, bool *setval) > +enum { > + Opt_uid, Opt_gid, Opt_umask, Opt_fmask, Opt_dmask, > + Opt_mft_zone_multiplier, Opt_sloppy_no, Opt_sloppy_yes, > + Opt_show_sys_files_no, Opt_show_sys_files_yes, > + Opt_case_sensitive_no, Opt_case_sensitive_yes, > + Opt_disable_sparse_no, Opt_disable_sparse_yes, > + Opt_errors_panic, Opt_errors_remount_ro, Opt_errors_continue, > + Opt_errors_recover, Opt_posix, Opt_show_inodes, Opt_nls, > + Opt_iocharset, Opt_utf8_no, Opt_utf8_yes, Opt_err > +}; > + > +static match_table_t tokens = { > + {Opt_uid, "uid=%u"}, > + {Opt_gid, "gid=%u"}, > + {Opt_umask, "umask=%o"}, > + {Opt_fmask, "fmask=%o"}, > + {Opt_dmask, "dmask=%o"}, > + {Opt_mft_zone_multiplier, "mft_zone_multiplier=%u"}, > + {Opt_sloppy_no, "sloppy=0"}, > + {Opt_sloppy_no, "sloppy=no"}, > + {Opt_sloppy_no, "sloppy=false"}, > + {Opt_sloppy_yes, "sloppy=1"}, > + {Opt_sloppy_yes, "sloppy=yes"}, > + {Opt_sloppy_yes, "sloppy=true"}, > + {Opt_show_sys_files_no, "show_sys_files=0"}, > + {Opt_show_sys_files_no, "show_sys_files=no"}, > + {Opt_show_sys_files_no, "show_sys_files=false"}, > + {Opt_show_sys_files_yes, "show_sys_files=1"}, > + {Opt_show_sys_files_yes, "show_sys_files=yes"}, > + {Opt_show_sys_files_yes, "show_sys_files=true"}, > + {Opt_case_sensitive_no, "case_sensitive=0"}, > + {Opt_case_sensitive_no, "case_sensitive=no"}, > + {Opt_case_sensitive_no, "case_sensitive=false"}, > + {Opt_case_sensitive_yes, "case_sensitive=1"}, > + {Opt_case_sensitive_yes, "case_sensitive=yes"}, > + {Opt_case_sensitive_yes, "case_sensitive=true"}, > + {Opt_disable_sparse_no, "disable_sparse=0"}, > + {Opt_disable_sparse_no, "disable_sparse=no"}, > + {Opt_disable_sparse_no, "disable_sparse=false"}, > + {Opt_disable_sparse_yes, "disable_sparse=1"}, > + {Opt_disable_sparse_yes, "disable_sparse=yes"}, > + {Opt_disable_sparse_yes, "disable_sparse=true"}, > + {Opt_errors_panic, "errors=panic"}, > + {Opt_errors_remount_ro, "errors=remount-ro"}, > + {Opt_errors_continue, "errors=continue"}, > + {Opt_errors_recover, "errors=recover"}, > + {Opt_posix, "posix=%s"}, /*obsolete*/ > + {Opt_show_inodes, "show_inodes=%s"}, /*obsolete*/ > + {Opt_nls, "nls=%s"}, > + {Opt_iocharset, "iocharset=%s"}, /*Deprecated*/ > + {Opt_utf8_no, "utf8=0"}, /*no longer supported*/ > + {Opt_utf8_no, "utf8=no"}, > + {Opt_utf8_no, "utf8=false"}, > + {Opt_utf8_yes, "utf8=1"}, > + {Opt_utf8_yes, "utf8=yes"}, > + {Opt_utf8_yes, "utf8=true"}, > + {Opt_utf8_yes, "utf8"}, > + {Opt_err, NULL} > +}; > + > + /** > + * load_nls_ntfs - load the nls map > + * @vol: ntfs volume > + * @nls_name: nls name > + * > + * Load the nls map which name is @nls_name. If load failed and > the @vol has > + * load the nls map before, use the old nls map. > + */ > +static struct nls_table *load_nls_ntfs(ntfs_volume *vol, char > *nls_name) > { > - if (s) { > - if (!strcmp(s, "1") || !strcmp(s, "yes") || !strcmp(s, "true")) > - *setval = true; > - else if (!strcmp(s, "0") || !strcmp(s, "no") || > - !strcmp(s, "false")) > - *setval = false; > - else > - return 0; > - } else > - *setval = true; > - return 1; > + struct nls_table *nls_map = NULL, *old_nls = vol->nls_map; > + > + nls_map = load_nls(nls_name); > + if (!nls_map) { > + if (!old_nls) { > + ntfs_error(vol->sb, "NLS character set " > + "%s not found.", nls_name); > + return NULL; > + } > + ntfs_error(vol->sb, "NLS character set %s not " > + "found. Using previous one %s.", > + nls_name, old_nls->charset); > + nls_map = old_nls; > + } > + return nls_map; > } > > /** > @@ -98,136 +166,137 @@ static int simple_getbool(char *s, bool > *setval) > */ > static bool parse_options(ntfs_volume *vol, char *opt) > { > - char *p, *v, *ov; > + char *p, *v; > static char *utf8 = "utf8"; > - int errors = 0, sloppy = 0; > + int errors = 0, sloppy = 0, mask = -1; > uid_t uid = (uid_t)-1; > gid_t gid = (gid_t)-1; > mode_t fmask = (mode_t)-1, dmask = (mode_t)-1; > int mft_zone_multiplier = -1, on_errors = -1; > int show_sys_files = -1, case_sensitive = -1, disable_sparse = -1; > - struct nls_table *nls_map = NULL, *old_nls; > - > - /* I am lazy... (-8 */ > -#define NTFS_GETOPT_WITH_DEFAULT(option, variable, default_value) \ > - if (!strcmp(p, option)) { \ > - if (!v || !*v) \ > - variable = default_value; \ > - else { \ > - variable = simple_strtoul(ov = v, &v, 0); \ > - if (*v) \ > - goto needs_val; \ > - } \ > - } > -#define NTFS_GETOPT(option, variable) \ > - if (!strcmp(p, option)) { \ > - if (!v || !*v) \ > - goto needs_arg; \ > - variable = simple_strtoul(ov = v, &v, 0); \ > - if (*v) \ > - goto needs_val; \ > - } > -#define NTFS_GETOPT_OCTAL(option, variable) \ > - if (!strcmp(p, option)) { \ > - if (!v || !*v) \ > - goto needs_arg; \ > - variable = simple_strtoul(ov = v, &v, 8); \ > - if (*v) \ > - goto needs_val; \ > - } > -#define NTFS_GETOPT_BOOL(option, variable) \ > - if (!strcmp(p, option)) { \ > - bool val; \ > - if (!simple_getbool(v, &val)) \ > - goto needs_bool; \ > - variable = val; \ > - } > -#define NTFS_GETOPT_OPTIONS_ARRAY(option, variable, opt_array) \ > - if (!strcmp(p, option)) { \ > - int _i; \ > - if (!v || !*v) \ > - goto needs_arg; \ > - ov = v; \ > - if (variable == -1) \ > - variable = 0; \ > - for (_i = 0; opt_array[_i].str && *opt_array[_i].str; _i++) \ > - if (!strcmp(opt_array[_i].str, v)) { \ > - variable |= opt_array[_i].val; \ > - break; \ > - } \ > - if (!opt_array[_i].str || !*opt_array[_i].str) \ > - goto needs_val; \ > - } > + struct nls_table *nls_map = NULL; > + substring_t args[MAX_OPT_ARGS]; > + int token; > + > if (!opt || !*opt) > goto no_mount_options; > ntfs_debug("Entering with mount options string: %s", opt); > while ((p = strsep(&opt, ","))) { > - if ((v = strchr(p, '='))) > - *v++ = 0; > - NTFS_GETOPT("uid", uid) > - else NTFS_GETOPT("gid", gid) > - else NTFS_GETOPT_OCTAL("umask", fmask = dmask) > - else NTFS_GETOPT_OCTAL("fmask", fmask) > - else NTFS_GETOPT_OCTAL("dmask", dmask) > - else NTFS_GETOPT("mft_zone_multiplier", mft_zone_multiplier) > - else NTFS_GETOPT_WITH_DEFAULT("sloppy", sloppy, true) > - else NTFS_GETOPT_BOOL("show_sys_files", show_sys_files) > - else NTFS_GETOPT_BOOL("case_sensitive", case_sensitive) > - else NTFS_GETOPT_BOOL("disable_sparse", disable_sparse) > - else NTFS_GETOPT_OPTIONS_ARRAY("errors", on_errors, > - on_errors_arr) > - else if (!strcmp(p, "posix") || !strcmp(p, "show_inodes")) > + if (!*p) > + continue; > + > + token = match_token(p, tokens, args); > + switch (token) { > + case Opt_uid: > + if (match_int(&args[0], &uid)) > + return false; > + break; > + case Opt_gid: > + if (match_int(&args[0], &gid)) > + return false; > + break; > + case Opt_umask: > + if (match_octal(&args[0], &mask)) > + return false; > + dmask = fmask = (mode_t)mask; > + break; > + case Opt_fmask: > + if (match_octal(&args[0], &mask)) > + return false; > + fmask = (mode_t)mask; > + break; > + case Opt_dmask: > + if (match_octal(&args[0], &mask)) > + return false; > + dmask = (mode_t)mask; > + break; > + case Opt_mft_zone_multiplier: > + if (match_int(&args[0], &mft_zone_multiplier)) > + return false; > + break; > + case Opt_sloppy_no: > + sloppy = false; > + break; > + case Opt_sloppy_yes: > + sloppy = true; > + break; > + case Opt_show_sys_files_no: > + show_sys_files = false; > + break; > + case Opt_show_sys_files_yes: > + show_sys_files = true; > + break; > + case Opt_case_sensitive_no: > + case_sensitive = false; > + break; > + case Opt_case_sensitive_yes: > + case_sensitive = true; > + break; > + case Opt_disable_sparse_no: > + disable_sparse = false; > + break; > + case Opt_disable_sparse_yes: > + disable_sparse = true; > + break; > + case Opt_errors_panic: > + if (on_errors == -1) > + on_errors = ON_ERRORS_PANIC; > + else > + on_errors |= ON_ERRORS_PANIC; > + break; > + case Opt_errors_remount_ro: > + if (on_errors == -1) > + on_errors = ON_ERRORS_REMOUNT_RO; > + else > + on_errors |= ON_ERRORS_REMOUNT_RO; > + break; > + case Opt_errors_continue: > + if (on_errors == -1) > + on_errors = ON_ERRORS_CONTINUE; > + else > + on_errors |= ON_ERRORS_CONTINUE; > + break; > + case Opt_errors_recover: > + if (on_errors == -1) > + on_errors = ON_ERRORS_RECOVER; > + else > + on_errors |= ON_ERRORS_RECOVER; > + break; > + case Opt_posix: > + case Opt_show_inodes: > ntfs_warning(vol->sb, "Ignoring obsolete option %s.", > - p); > - else if (!strcmp(p, "nls") || !strcmp(p, "iocharset")) { > - if (!strcmp(p, "iocharset")) > - ntfs_warning(vol->sb, "Option iocharset is " > - "deprecated. Please use " > - "option nls= in " > - "the future."); > - if (!v || !*v) > - goto needs_arg; > -use_utf8: > - old_nls = nls_map; > - nls_map = load_nls(v); > - if (!nls_map) { > - if (!old_nls) { > - ntfs_error(vol->sb, "NLS character set " > - "%s not found.", v); > - return false; > - } > - ntfs_error(vol->sb, "NLS character set %s not " > - "found. Using previous one %s.", > - v, old_nls->charset); > - nls_map = old_nls; > - } else /* nls_map */ { > - if (old_nls) > - unload_nls(old_nls); > - } > - } else if (!strcmp(p, "utf8")) { > - bool val = false; > + p); > + break; > + case Opt_iocharset: > + ntfs_warning(vol->sb, "Option iocharset is " > + "deprecated. Please use " > + "option nls= in " > + "the future."); > + case Opt_nls: > + v = match_strdup(&args[0]); > + if (!v) > + return false; > + nls_map = load_nls_ntfs(vol, v); > + kfree(v); > + break; > + case Opt_utf8_yes: > + case Opt_utf8_no: > ntfs_warning(vol->sb, "Option utf8 is no longer " > - "supported, using option nls=utf8. Please " > - "use option nls=utf8 in the future and " > - "make sure utf8 is compiled either as a " > - "module or into the kernel."); > - if (!v || !*v) > - val = true; > - else if (!simple_getbool(v, &val)) > - goto needs_bool; > - if (val) { > - v = utf8; > - goto use_utf8; > - } > - } else { > - ntfs_error(vol->sb, "Unrecognized mount option %s.", p); > + "supported, using option nls=utf8. Please " > + "use option nls=utf8 in the future and " > + "make sure utf8 is compiled either as a " > + "module or into the kernel."); > + if (token == Opt_utf8_no) > + break; > + nls_map = load_nls_ntfs(vol, utf8); > + break; > + default: /* Opt_err */ > + ntfs_error(vol->sb, "Unrecognized or incorrect " > + "mount option %s.", p); > if (errors < INT_MAX) > errors++; > + break; > } > -#undef NTFS_GETOPT_OPTIONS_ARRAY > -#undef NTFS_GETOPT_BOOL > -#undef NTFS_GETOPT > -#undef NTFS_GETOPT_WITH_DEFAULT > } > no_mount_options: > if (errors && !sloppy) > @@ -319,15 +388,6 @@ no_mount_options: > } > } > return true; > -needs_arg: > - ntfs_error(vol->sb, "The %s option requires an argument.", p); > - return false; > -needs_bool: > - ntfs_error(vol->sb, "The %s option requires a boolean argument.", > p); > - return false; > -needs_val: > - ntfs_error(vol->sb, "Invalid %s option argument: %s", p, ov); > - return false; > } > > #ifdef NTFS_RW Best regards, Anton -- Anton Altaparmakov (replace at with @) Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK Linux NTFS maintainer, http://www.linux-ntfs.org/ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/