Re: A patch to implement the already documented "sep" option for the CIFS file system.

[Enzo Matsumiya <ematsumiya@suse.de>]

On 10/11, Enzo Matsumiya wrote:
>On 10/11, Steve French wrote:
>>All three approaches seem to parse it in user space, which makes
>>sense.  Easier to do it in mount.cifs than in kernel fs_context
>>parsing
>
>Yes, I'm just saying that the original approach (having a 'sep=' option)
>would require implementation in the kernel as well (it currently doesn't
>exist).  If we just replace the comma by 0x1E in both mount.cifs and the
>kernel, we don't need all this fiddling with custom separators.

Hastily hacked PoC patches follow.  Worked with my simple test:

   mount.cifs -o vers=3.1.1,sign,credentials=/root/sambacreds "//192.168.0.15/my, share" /mnt/samba


Enzo

Kernel patch:

--- a/fs/cifs/fs_context.c
+++ b/fs/cifs/fs_context.c
@@ -414,7 +414,7 @@ int smb3_parse_opt(const char *options, const char *key, char **val)
  	if (!opts)
  		return -ENOMEM;
  
-	while ((p = strsep(&opts, ","))) {
+	while ((p = strsep(&opts, "\x1e"))) {
  		char *nval;
  
  		if (!*p)
@@ -585,7 +585,7 @@ static int smb3_fs_context_parse_monolithic(struct fs_context *fc,
  		return ret;
  
  	/* BB Need to add support for sep= here TBD */
-	while ((key = strsep(&options, ",")) != NULL) {
+	while ((key = strsep(&options, "\x1e")) != NULL) {
  		size_t len;
  		char *value;


cifs-utils patch:
  
diff --git a/mount.cifs.c b/mount.cifs.c
index 2278995c9653..1d48e257a794 100644
--- a/mount.cifs.c
+++ b/mount.cifs.c
@@ -1195,7 +1195,7 @@ parse_options(const char *data, struct parsed_mount_info *parsed_info)
  
  		/* go ahead and copy */
  		if (out_len)
-			strlcat(out, ",", MAX_OPTIONS_LEN);
+			strlcat(out, "\x1e", MAX_OPTIONS_LEN);
  
  		strlcat(out, data, MAX_OPTIONS_LEN);
  		out_len = strlen(out);
@@ -1215,7 +1215,7 @@ nocopy:
  		}
  
  		if (out_len) {
-			strlcat(out, ",", MAX_OPTIONS_LEN);
+			strlcat(out, "\x1e", MAX_OPTIONS_LEN);
  			out_len++;
  		}
  		snprintf(out + out_len, word_len + 5, "uid=%s", txtbuf);
@@ -1235,7 +1235,7 @@ nocopy:
  		}
  
  		if (out_len) {
-			strlcat(out, ",", MAX_OPTIONS_LEN);
+			strlcat(out, "\x1e", MAX_OPTIONS_LEN);
  			out_len++;
  		}
  		snprintf(out + out_len, word_len + 7, "cruid=%s", txtbuf);
@@ -1251,7 +1251,7 @@ nocopy:
  		}
  
  		if (out_len) {
-			strlcat(out, ",", MAX_OPTIONS_LEN);
+			strlcat(out, "\x1e", MAX_OPTIONS_LEN);
  			out_len++;
  		}
  		snprintf(out + out_len, word_len + 5, "gid=%s", txtbuf);
@@ -1267,7 +1267,7 @@ nocopy:
  		}
  
  		if (out_len) {
-			strlcat(out, ",", MAX_OPTIONS_LEN);
+			strlcat(out, "\x1e", MAX_OPTIONS_LEN);
  			out_len++;
  		}
  		snprintf(out + out_len, word_len + 11, "backupuid=%s", txtbuf);
@@ -1283,7 +1283,7 @@ nocopy:
  		}
  
  		if (out_len) {
-			strlcat(out, ",", MAX_OPTIONS_LEN);
+			strlcat(out, "\x1e", MAX_OPTIONS_LEN);
  			out_len++;
  		}
  		snprintf(out + out_len, word_len + 11, "backupgid=%s", txtbuf);
@@ -1299,7 +1299,7 @@ nocopy:
  		}
  
  		if (out_len) {
-			strlcat(out, ",", MAX_OPTIONS_LEN);
+			strlcat(out, "\x1e", MAX_OPTIONS_LEN);
  			out_len++;
  		}
  		snprintf(out + out_len, word_len + 10, "snapshot=%s", txtbuf);
@@ -1558,17 +1558,17 @@ add_mtab(char *devname, char *mountpoint, unsigned long flags, const char *fstyp
  			strlcat(mountent.mnt_opts, "rw", MTAB_OPTIONS_LEN);
  
  		if (flags & MS_MANDLOCK)
-			strlcat(mountent.mnt_opts, ",mand", MTAB_OPTIONS_LEN);
+			strlcat(mountent.mnt_opts, "\x1emand", MTAB_OPTIONS_LEN);
  		if (flags & MS_NOEXEC)
-			strlcat(mountent.mnt_opts, ",noexec", MTAB_OPTIONS_LEN);
+			strlcat(mountent.mnt_opts, "\x1enoexec", MTAB_OPTIONS_LEN);
  		if (flags & MS_NOSUID)
-			strlcat(mountent.mnt_opts, ",nosuid", MTAB_OPTIONS_LEN);
+			strlcat(mountent.mnt_opts, "\x1enosuid", MTAB_OPTIONS_LEN);
  		if (flags & MS_NODEV)
-			strlcat(mountent.mnt_opts, ",nodev", MTAB_OPTIONS_LEN);
+			strlcat(mountent.mnt_opts, "\x1enodev", MTAB_OPTIONS_LEN);
  		if (flags & MS_SYNCHRONOUS)
-			strlcat(mountent.mnt_opts, ",sync", MTAB_OPTIONS_LEN);
+			strlcat(mountent.mnt_opts, "\x1esync", MTAB_OPTIONS_LEN);
  		if (mount_user) {
-			strlcat(mountent.mnt_opts, ",user=", MTAB_OPTIONS_LEN);
+			strlcat(mountent.mnt_opts, "\x1euser=", MTAB_OPTIONS_LEN);
  			strlcat(mountent.mnt_opts, mount_user,
  				MTAB_OPTIONS_LEN);
  		}
@@ -1942,7 +1942,7 @@ assemble_mountinfo(struct parsed_mount_info *parsed_info,
  	/* copy in user= string */
  	if (parsed_info->got_user) {
  		if (*parsed_info->options)
-			strlcat(parsed_info->options, ",",
+			strlcat(parsed_info->options, "\x1e",
  				sizeof(parsed_info->options));
  		strlcat(parsed_info->options, "user=",
  			sizeof(parsed_info->options));
@@ -1952,14 +1952,14 @@ assemble_mountinfo(struct parsed_mount_info *parsed_info,
  
  	if (*parsed_info->domain) {
  		if (*parsed_info->options)
-			strlcat(parsed_info->options, ",",
+			strlcat(parsed_info->options, "\x1e",
  				sizeof(parsed_info->options));
  		strlcat(parsed_info->options, "domain=",
  			sizeof(parsed_info->options));
  		strlcat(parsed_info->options, parsed_info->domain,
  			sizeof(parsed_info->options));
  	} else if (parsed_info->got_domain) {
-		strlcat(parsed_info->options, ",domain=",
+		strlcat(parsed_info->options, "\x1e" "domain=",
  			sizeof(parsed_info->options));
  	}
  
@@ -2216,23 +2216,23 @@ mount_retry:
  	strlcpy(options, "ip=", options_size);
  	strlcat(options, currentaddress, options_size);
  
-	strlcat(options, ",unc=\\\\", options_size);
+	strlcat(options, "\x1eunc=\\\\", options_size);
  	strlcat(options, parsed_info->host, options_size);
  	strlcat(options, "\\", options_size);
  	strlcat(options, parsed_info->share, options_size);
  
  	if (*parsed_info->options) {
-		strlcat(options, ",", options_size);
+		strlcat(options, "\x1e", options_size);
  		strlcat(options, parsed_info->options, options_size);
  	}
  
  	if (*parsed_info->prefix) {
-		strlcat(options, ",prefixpath=", options_size);
+		strlcat(options, "\x1eprefixpath=", options_size);
  		strlcat(options, parsed_info->prefix, options_size);
  	}
  
  	if (sloppy)
-		strlcat(options, ",sloppy", options_size);
+		strlcat(options, "\x1esloppy", options_size);
  
  	if (parsed_info->verboseflag)
  		fprintf(stderr, "%s kernel mount options: %s",
@@ -2243,10 +2243,10 @@ mount_retry:
  		 * Commas have to be doubled, or else they will
  		 * look like the parameter separator
  		 */
-		strlcat(options, ",pass=", options_size);
+		strlcat(options, "\x1epass=", options_size);
  		strlcat(options, parsed_info->password, options_size);
  		if (parsed_info->verboseflag)
-			fprintf(stderr, ",pass=********");
+			fprintf(stderr, "\x1epass=********");
  	}
  
  	if (parsed_info->verboseflag)