[linux-cifs-client] [PATCH] Clean up option parsing in mount.cifs
Jeff Layton
jlayton at samba.org
Tue May 11 08:05:03 MDT 2010
On Thu, 29 Apr 2010 09:58:16 -0400
Scott Lovenberg <scott.lovenberg at gmail.com> wrote:
> Moved option string parsing to function parse_opt_token(char*).
> Main loop in parse_options(const char*, struct parsed_mount_info*) transplanted to a switch block.
>
> The parsing function folds common options to a single macro:
> 1.) 'unc','target', and 'path' -> 'OPT_UNC'
> 2.) 'dom*' and 'workg*' -> 'OPT_DOM'
> 3.) 'nobrl' and 'nolock' -> 'OPT_NO_LOCK'
>
> Kept 'fmask' and 'dmask' (OPT_FMASK, OPT_DMASK), which fall through to 'file_mode' and 'dir_mode' in the main loop.
>
> Signed-off-by: Scott Lovenberg <scott.lovenberg at gmail.com>
> ---
> mount.cifs.c | 263 +++++++++++++++++++++++++++++++++++++++++----------------
> 1 files changed, 189 insertions(+), 74 deletions(-)
>
Looks like a definite improvement overall, but gcc complains:
mount.cifs.c: In function ‘parse_options’:
mount.cifs.c:843: warning: passing argument 1 of ‘parse_opt_token’ discards qualifiers from pointer target type
mount.cifs.c:733: note: expected ‘char *’ but argument is of type ‘const char *’
...you can probably fix this by making parse_opt_token take a "const
char *" rather than a "char *". It's not altered so that should be safe.
> diff --git a/mount.cifs.c b/mount.cifs.c
> index e621178..155d594 100644
> --- a/mount.cifs.c
> +++ b/mount.cifs.c
> @@ -129,6 +129,38 @@
> #define CRED_PASS 2
> #define CRED_DOM 4
>
> +/*
> + * Values for parsing command line options.
> + */
> +#define OPT_ERROR -1
> +#define OPT_USERS 1
> +#define OPT_USER 2
> +#define OPT_USER_XATTR 3
> +#define OPT_PASS 4
> +#define OPT_SEC 5
> +#define OPT_IP 6
> +#define OPT_UNC 7
> +#define OPT_CRED 8
> +#define OPT_UID 9
> +#define OPT_GID 10
> +#define OPT_FMASK 11
> +#define OPT_FILE_MODE 12
> +#define OPT_DMASK 13
> +#define OPT_DIR_MODE 14
> +#define OPT_DOM 15
> +#define OPT_NO_SUID 16
> +#define OPT_SUID 17
> +#define OPT_NO_DEV 18
> +#define OPT_DEV 19
> +#define OPT_NO_LOCK 20
> +#define OPT_NO_EXEC 21
> +#define OPT_EXEC 22
> +#define OPT_GUEST 23
> +#define OPT_RO 24
> +#define OPT_RW 25
> +#define OPT_REMOUNT 26
> +
> +
Minor nit -- it's easier to read large lists of macros like this if
they are in columns. For instance:
#define OPT_ERROR -1
#define OPT_USERS 1
#define OPT_USER 2
Since you need to fix the compiler warning, might as well fix this
too...
> /* struct for holding parsed mount info for use by privleged process */
> struct parsed_mount_info {
> unsigned long flags;
> @@ -694,17 +726,83 @@ get_pw_exit:
> return rc;
> }
>
> +/*
> + * Returns OPT_ERROR on unparsable token.
> + */
> +static int parse_opt_token(char *token)
> +{
> + if (token == NULL)
> + return OPT_ERROR;
> +
> + if (strncmp(token, "users", 5) == 0)
> + return OPT_USERS;
> + if (strncmp(token, "user_xattr", 10) == 0)
> + return OPT_USER_XATTR;
> + if (strncmp(token, "user", 4) == 0)
> + return OPT_USER;
> + if (strncmp(token, "pass", 4) == 0)
> + return OPT_PASS;
> + if (strncmp(token, "sec", 3) == 0)
> + return OPT_SEC;
> + if (strncmp(token, "ip", 2) == 0)
> + return OPT_IP;
> + if (strncmp(token, "unc", 3) == 0 ||
> + strncmp(token, "target", 6) == 0 ||
> + strncmp(token, "path", 4) == 0)
> + return OPT_UNC;
> + if (strncmp(token, "dom", 3) == 0 || strncmp(token, "workg", 5) == 0)
> + return OPT_DOM;
> + if (strncmp(token, "uid", 3) == 0)
> + return OPT_UID;
> + if (strncmp(token, "gid", 3) == 0)
> + return OPT_GID;
> + if (strncmp(token, "fmask", 5) == 0)
> + return OPT_FMASK;
> + if (strncmp(token, "file_mode", 9) == 0)
> + return OPT_FILE_MODE;
> + if (strncmp(token, "dmask", 5) == 0)
> + return OPT_DMASK;
> + if (strncmp(token, "dir_mode", 8) == 0)
> + return OPT_DIR_MODE;
> + if (strncmp(token, "nosuid", 6) == 0)
> + return OPT_NO_SUID;
> + if (strncmp(token, "suid", 4) == 0)
> + return OPT_SUID;
> + if (strncmp(token, "nodev", 5) == 0)
> + return OPT_NO_DEV;
> + if (strncmp(token, "nobrl", 5) == 0 || strncmp(token, "nolock", 6) == 0)
> + return OPT_NO_LOCK;
> + if (strncmp(token, "dev", 3) == 0)
> + return OPT_DEV;
> + if (strncmp(token, "noexec", 6) == 0)
> + return OPT_NO_EXEC;
> + if (strncmp(token, "exec", 4) == 0)
> + return OPT_EXEC;
> + if (strncmp(token, "guest", 5) == 0)
> + return OPT_GUEST;
> + if (strncmp(token, "ro", 2) == 0)
> + return OPT_RO;
> + if (strncmp(token, "rw", 2) == 0)
> + return OPT_RW;
> + if (strncmp(token, "remount", 7) == 0)
> + return OPT_REMOUNT;
> +
> + return OPT_ERROR;
> +}
> +
> static int
> parse_options(const char *data, struct parsed_mount_info *parsed_info)
> {
> - char *value = NULL, *equals = NULL;
> + char *value = NULL;
> + char *equals = NULL;
> char *next_keyword = NULL;
> char *out = parsed_info->options;
> unsigned long *filesys_flags = &parsed_info->flags;
> int out_len = 0;
> int word_len;
> int rc = 0;
> - int got_uid = 0, got_gid = 0;
> + int got_uid = 0;
> + int got_gid = 0;
> char user[32];
> char group[32];
>
> @@ -741,15 +839,15 @@ parse_options(const char *data, struct parsed_mount_info *parsed_info)
> value = equals + 1;
> }
>
> - /* FIXME: turn into a token parser? */
> - if (strncmp(data, "users", 5) == 0) {
> + switch(parse_opt_token(data)) {
> + case OPT_USERS:
> if (!value || !*value) {
> *filesys_flags |= MS_USERS;
> goto nocopy;
> }
> - } else if (strncmp(data, "user_xattr", 10) == 0) {
> - /* do nothing - need to skip so not parsed as user name */
> - } else if (strncmp(data, "user", 4) == 0) {
> + break;
> +
> + case OPT_USER:
> if (!value || !*value) {
> if (data[4] == '\0') {
> *filesys_flags |= MS_USER;
> @@ -772,7 +870,8 @@ parse_options(const char *data, struct parsed_mount_info *parsed_info)
> }
> goto nocopy;
> }
> - } else if (strncmp(data, "pass", 4) == 0) {
> +
> + case OPT_PASS:
> if (parsed_info->got_password) {
> fprintf(stderr,
> "password specified twice, ignoring second\n");
> @@ -786,18 +885,21 @@ parse_options(const char *data, struct parsed_mount_info *parsed_info)
> if (rc)
> return rc;
> goto nocopy;
> - } else if (strncmp(data, "sec", 3) == 0) {
> +
> + case OPT_SEC:
> if (value) {
> if (!strncmp(value, "none", 4) ||
> !strncmp(value, "krb5", 4))
> parsed_info->got_password = 1;
> }
> - } else if (strncmp(data, "ip", 2) == 0) {
> + break;
> +
> + case OPT_IP:
> if (!value || !*value) {
> fprintf(stderr,
> - "target ip address argument missing");
> + "target ip address argument missing\n");
> } else if (strnlen(value, MAX_ADDRESS_LEN) <=
> - MAX_ADDRESS_LEN) {
> + MAX_ADDRESS_LEN) {
> if (parsed_info->verboseflag)
> fprintf(stderr,
> "ip address %s override specified\n",
> @@ -805,23 +907,24 @@ parse_options(const char *data, struct parsed_mount_info *parsed_info)
> } else {
> fprintf(stderr, "ip address too long\n");
> return EX_USAGE;
> +
> }
> - } else if ((strncmp(data, "unc", 3) == 0)
> - || (strncmp(data, "target", 6) == 0)
> - || (strncmp(data, "path", 4) == 0)) {
> + break;
> +
> + /* unc || target || path */
> + case OPT_UNC:
> if (!value || !*value) {
> fprintf(stderr,
> "invalid path to network resource\n");
> - return EX_USAGE; /* needs_arg; */
> + return EX_USAGE;
> }
> rc = parse_unc(value, parsed_info);
> if (rc)
> return rc;
> - } else if ((strncmp(data, "dom" /* domain */ , 3) == 0)
> - || (strncmp(data, "workg", 5) == 0)) {
> - /* note this allows for synonyms of "domain"
> - such as "DOM" and "dom" and "workgroup"
> - and "WORKGRP" etc. */
> + break;
> +
> + /* dom || workgroup */
> + case OPT_DOM:
> if (!value || !*value) {
> fprintf(stderr, "CIFS: invalid domain name\n");
> return EX_USAGE;
> @@ -834,21 +937,23 @@ parse_options(const char *data, struct parsed_mount_info *parsed_info)
> strlcpy(parsed_info->domain, value,
> sizeof(parsed_info->domain));
> goto nocopy;
> - } else if (strncmp(data, "cred", 4) == 0) {
> - if (value && *value) {
> - rc = open_cred_file(value, parsed_info);
> - if (rc) {
> - fprintf(stderr,
> - "error %d (%s) opening credential file %s\n",
> - rc, strerror(rc), value);
> - return rc;
> - }
> - } else {
> +
> + case OPT_CRED:
> + if (!value || !*value) {
> fprintf(stderr,
> "invalid credential file name specified\n");
> return EX_USAGE;
> }
> - } else if (strncmp(data, "uid", 3) == 0) {
> + rc = open_cred_file(value, parsed_info);
> + if (rc) {
> + fprintf(stderr,
> + "error %d (%s) opening credential file %s\n",
> + rc, strerror(rc), value);
> + return rc;
> + }
> + break;
> +
> + case OPT_UID:
> if (value && *value) {
> got_uid = 1;
> if (!isdigit(*value)) {
> @@ -862,12 +967,13 @@ parse_options(const char *data, struct parsed_mount_info *parsed_info)
> }
> snprintf(user, sizeof(user), "%u",
> pw->pw_uid);
> - } else {
> - strlcpy(user, value, sizeof(user));
> }
> + else
> + strlcpy(user, value, sizeof(user));
> }
> goto nocopy;
> - } else if (strncmp(data, "gid", 3) == 0) {
> +
> + case OPT_GID:
> if (value && *value) {
> got_gid = 1;
> if (!isdigit(*value)) {
> @@ -881,14 +987,19 @@ parse_options(const char *data, struct parsed_mount_info *parsed_info)
> }
> snprintf(group, sizeof(group), "%u",
> gr->gr_gid);
> - } else {
> - strlcpy(group, value, sizeof(group));
> }
> + else
> + strlcpy(group, value, sizeof(group));
> }
> goto nocopy;
> - /* fmask and dmask synonyms for people used to smbfs syntax */
> - } else if (strcmp(data, "file_mode") == 0
> - || strcmp(data, "fmask") == 0) {
> +
> + /* fmask fall through to file_mode */
> + case OPT_FMASK:
> + fprintf(stderr,
> + "WARNING: CIFS mount option 'fmask' is\
> + deprecated. Use 'file_mode' instead.\n");
> + data = "file_mode"; /* BB fix this */
> + case OPT_FILE_MODE:
> if (!value || !*value) {
> fprintf(stderr,
> "Option '%s' requires a numerical argument\n",
> @@ -896,19 +1007,19 @@ parse_options(const char *data, struct parsed_mount_info *parsed_info)
> return EX_USAGE;
> }
>
> - if (value[0] != '0') {
> + if (value[0] != '0')
> fprintf(stderr,
> "WARNING: '%s' not expressed in octal.\n",
> data);
> - }
> + break;
>
> - if (strcmp(data, "fmask") == 0) {
> - fprintf(stderr,
> - "WARNING: CIFS mount option 'fmask' is deprecated. Use 'file_mode' instead.\n");
> - data = "file_mode"; /* BB fix this */
> - }
> - } else if (strcmp(data, "dir_mode") == 0
> - || strcmp(data, "dmask") == 0) {
> + /* dmask falls through to dir_mode */
> + case OPT_DMASK:
> + fprintf(stderr,
> + "WARNING: CIFS mount option 'dmask' is\
> + deprecated. Use 'dir_mode' instead.\n");
> + data = "dir_mode";
> + case OPT_DIR_MODE:
> if (!value || !*value) {
> fprintf(stderr,
> "Option '%s' requires a numerical argument\n",
> @@ -916,49 +1027,53 @@ parse_options(const char *data, struct parsed_mount_info *parsed_info)
> return EX_USAGE;
> }
>
> - if (value[0] != '0') {
> + if (value[0] != '0')
> fprintf(stderr,
> "WARNING: '%s' not expressed in octal.\n",
> data);
> - }
> + break;
>
> - if (strcmp(data, "dmask") == 0) {
> - fprintf(stderr,
> - "WARNING: CIFS mount option 'dmask' is deprecated. Use 'dir_mode' instead.\n");
> - data = "dir_mode";
> - }
> - /* the following eight mount options should be
> - stripped out from what is passed into the kernel
> - since these eight options are best passed as the
> - mount flags rather than redundantly to the kernel
> - and could generate spurious warnings depending on the
> - level of the corresponding cifs vfs kernel code */
> - } else if (strncmp(data, "nosuid", 6) == 0) {
> + /* the following mount options should be
> + stripped out from what is passed into the kernel
> + since these options are best passed as the
> + mount flags rather than redundantly to the kernel
> + and could generate spurious warnings depending on the
> + level of the corresponding cifs vfs kernel code */
> + case OPT_NO_SUID:
> *filesys_flags |= MS_NOSUID;
> - } else if (strncmp(data, "suid", 4) == 0) {
> + break;
> + case OPT_SUID:
> *filesys_flags &= ~MS_NOSUID;
> - } else if (strncmp(data, "nodev", 5) == 0) {
> + break;
> + case OPT_NO_DEV:
> *filesys_flags |= MS_NODEV;
> - } else if ((strncmp(data, "nobrl", 5) == 0) ||
> - (strncmp(data, "nolock", 6) == 0)) {
> + break;
> + /* nolock || nobrl */
> + case OPT_NO_LOCK:
> *filesys_flags &= ~MS_MANDLOCK;
> - } else if (strncmp(data, "dev", 3) == 0) {
> + break;
> + case OPT_DEV:
> *filesys_flags &= ~MS_NODEV;
> - } else if (strncmp(data, "noexec", 6) == 0) {
> + break;
> + case OPT_NO_EXEC:
> *filesys_flags |= MS_NOEXEC;
> - } else if (strncmp(data, "exec", 4) == 0) {
> + break;
> + case OPT_EXEC:
> *filesys_flags &= ~MS_NOEXEC;
> - } else if (strncmp(data, "guest", 5) == 0) {
> + break;
> + case OPT_GUEST:
> parsed_info->got_user = 1;
> parsed_info->got_password = 1;
> - } else if (strncmp(data, "ro", 2) == 0) {
> + break;
> + case OPT_RO:
> *filesys_flags |= MS_RDONLY;
> goto nocopy;
> - } else if (strncmp(data, "rw", 2) == 0) {
> + case OPT_RW:
> *filesys_flags &= ~MS_RDONLY;
> goto nocopy;
> - } else if (strncmp(data, "remount", 7) == 0) {
> + case OPT_REMOUNT:
> *filesys_flags |= MS_REMOUNT;
> + break;
> }
>
> /* check size before copying option to buffer */
--
Jeff Layton <jlayton at samba.org>
More information about the linux-cifs-client
mailing list