[PATCH] cifs: call strtobool instead of custom implementation

[Andy Shevchenko]

Signed-off-by: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/***@public.gmane.org>
---
fs/cifs/cifs_debug.c | 51 +++++++++++++++++++++++++++++----------------------
1 file changed, 29 insertions(+), 22 deletions(-)

diff --git a/fs/cifs/cifs_debug.c b/fs/cifs/cifs_debug.c
index f3ac415..fa78b68 100644
--- a/fs/cifs/cifs_debug.c
+++ b/fs/cifs/cifs_debug.c
@@ -274,6 +274,7 @@ static ssize_t cifs_stats_proc_write(struct file *file,
const char __user *buffer, size_t count, loff_t *ppos)
{
char c;
+ bool bv;
int rc;
struct list_head *tmp1, *tmp2, *tmp3;
struct TCP_Server_Info *server;
@@ -284,7 +285,7 @@ static ssize_t cifs_stats_proc_write(struct file *file,
if (rc)
return rc;

- if (c == '1' || c == 'y' || c == 'Y' || c == '0') {
+ if (strtobool(&c, &bv) == 0) {
#ifdef CONFIG_CIFS_STATS2
atomic_set(&totBufAllocCount, 0);
atomic_set(&totSmBufAllocCount, 0);
@@ -451,15 +452,14 @@ static ssize_t cifsFYI_proc_write(struct file *file, const char __user *buffer,
size_t count, loff_t *ppos)
{
char c;
+ bool bv;
int rc;

rc = get_user(c, buffer);
if (rc)
return rc;
- if (c == '0' || c == 'n' || c == 'N')
- cifsFYI = 0;
- else if (c == '1' || c == 'y' || c == 'Y')
- cifsFYI = 1;
+ if (strtobool(&c, &bv) == 0)
+ cifsFYI = bv;
else if ((c > '1') && (c <= '9'))
cifsFYI = (int) (c - '0'); /* see cifs_debug.h for meanings */

@@ -490,15 +490,18 @@ static ssize_t cifs_linux_ext_proc_write(struct file *file,
const char __user *buffer, size_t count, loff_t *ppos)
{
char c;
+ bool bv;
int rc;

rc = get_user(c, buffer);
if (rc)
return rc;
- if (c == '0' || c == 'n' || c == 'N')
- linuxExtEnabled = 0;
- else if (c == '1' || c == 'y' || c == 'Y')
- linuxExtEnabled = 1;
+
+ rc = strtobool(&c, &bv);
+ if (rc)
+ return rc;
+
+ linuxExtEnabled = bv;

return count;
}
@@ -527,15 +530,18 @@ static ssize_t cifs_lookup_cache_proc_write(struct file *file,
const char __user *buffer, size_t count, loff_t *ppos)
{
char c;
+ bool bv;
int rc;

rc = get_user(c, buffer);
if (rc)
return rc;
- if (c == '0' || c == 'n' || c == 'N')
- lookupCacheEnabled = 0;
- else if (c == '1' || c == 'y' || c == 'Y')
- lookupCacheEnabled = 1;
+
+ rc = strtobool(&c, &bv);
+ if (rc)
+ return rc;
+
+ lookupCacheEnabled = bv;

return count;
}
@@ -564,15 +570,18 @@ static ssize_t traceSMB_proc_write(struct file *file, const char __user *buffer,
size_t count, loff_t *ppos)
{
char c;
+ bool bv;
int rc;

rc = get_user(c, buffer);
if (rc)
return rc;
- if (c == '0' || c == 'n' || c == 'N')
- traceSMB = 0;
- else if (c == '1' || c == 'y' || c == 'Y')
- traceSMB = 1;
+
+ rc = strtobool(&c, &bv);
+ if (rc)
+ return rc;
+
+ traceSMB = bv;

return count;
}
@@ -630,6 +639,7 @@ static ssize_t cifs_security_flags_proc_write(struct file *file,
unsigned int flags;
char flags_string[12];
char c;
+ bool bv;

if ((count < 1) || (count > 11))
return -EINVAL;
@@ -642,11 +652,8 @@ static ssize_t cifs_security_flags_proc_write(struct file *file,
if (count < 3) {
/* single char or single char followed by null */
c = flags_string[0];
- if (c == '0' || c == 'n' || c == 'N') {
- global_secflags = CIFSSEC_DEF; /* default */
- return count;
- } else if (c == '1' || c == 'y' || c == 'Y') {
- global_secflags = CIFSSEC_MAX;
+ if (strtobool(&c, &bv) == 0) {
+ global_secflags = bv ? CIFSSEC_MAX : CIFSSEC_DEF;
return count;
} else if (!isdigit(c)) {
cifs_dbg(VFS, "Invalid SecurityFlags: %s\n",

[Steve French]

I don't have a strong opinion on whether this change makes it more or
less readable. Any other opinions?

On Tue, Apr 29, 2014 at 9:53 AM, Andy Shevchenko

Post by Andy Shevchenko
---
fs/cifs/cifs_debug.c | 51 +++++++++++++++++++++++++++++----------------------
1 file changed, 29 insertions(+), 22 deletions(-)
diff --git a/fs/cifs/cifs_debug.c b/fs/cifs/cifs_debug.c
index f3ac415..fa78b68 100644
--- a/fs/cifs/cifs_debug.c
+++ b/fs/cifs/cifs_debug.c
@@ -274,6 +274,7 @@ static ssize_t cifs_stats_proc_write(struct file *file,
const char __user *buffer, size_t count, loff_t *ppos)
{
char c;
+ bool bv;
int rc;
struct list_head *tmp1, *tmp2, *tmp3;
struct TCP_Server_Info *server;
@@ -284,7 +285,7 @@ static ssize_t cifs_stats_proc_write(struct file *file,
if (rc)
return rc;
- if (c == '1' || c == 'y' || c == 'Y' || c == '0') {
+ if (strtobool(&c, &bv) == 0) {
#ifdef CONFIG_CIFS_STATS2
atomic_set(&totBufAllocCount, 0);
atomic_set(&totSmBufAllocCount, 0);
@@ -451,15 +452,14 @@ static ssize_t cifsFYI_proc_write(struct file *file, const char __user *buffer,
size_t count, loff_t *ppos)
{
char c;
+ bool bv;
int rc;
rc = get_user(c, buffer);
if (rc)
return rc;
- if (c == '0' || c == 'n' || c == 'N')
- cifsFYI = 0;
- else if (c == '1' || c == 'y' || c == 'Y')
- cifsFYI = 1;
+ if (strtobool(&c, &bv) == 0)
+ cifsFYI = bv;
else if ((c > '1') && (c <= '9'))
cifsFYI = (int) (c - '0'); /* see cifs_debug.h for meanings */
@@ -490,15 +490,18 @@ static ssize_t cifs_linux_ext_proc_write(struct file *file,
const char __user *buffer, size_t count, loff_t *ppos)
{
char c;
+ bool bv;
int rc;
rc = get_user(c, buffer);
if (rc)
return rc;
- if (c == '0' || c == 'n' || c == 'N')
- linuxExtEnabled = 0;
- else if (c == '1' || c == 'y' || c == 'Y')
- linuxExtEnabled = 1;
+
+ rc = strtobool(&c, &bv);
+ if (rc)
+ return rc;
+
+ linuxExtEnabled = bv;
return count;
}
@@ -527,15 +530,18 @@ static ssize_t cifs_lookup_cache_proc_write(struct file *file,
const char __user *buffer, size_t count, loff_t *ppos)
{
char c;
+ bool bv;
int rc;
rc = get_user(c, buffer);
if (rc)
return rc;
- if (c == '0' || c == 'n' || c == 'N')
- lookupCacheEnabled = 0;
- else if (c == '1' || c == 'y' || c == 'Y')
- lookupCacheEnabled = 1;
+
+ rc = strtobool(&c, &bv);
+ if (rc)
+ return rc;
+
+ lookupCacheEnabled = bv;
return count;
}
@@ -564,15 +570,18 @@ static ssize_t traceSMB_proc_write(struct file *file, const char __user *buffer,
size_t count, loff_t *ppos)
{
char c;
+ bool bv;
int rc;
rc = get_user(c, buffer);
if (rc)
return rc;
- if (c == '0' || c == 'n' || c == 'N')
- traceSMB = 0;
- else if (c == '1' || c == 'y' || c == 'Y')
- traceSMB = 1;
+
+ rc = strtobool(&c, &bv);
+ if (rc)
+ return rc;
+
+ traceSMB = bv;
return count;
}
@@ -630,6 +639,7 @@ static ssize_t cifs_security_flags_proc_write(struct file *file,
unsigned int flags;
char flags_string[12];
char c;
+ bool bv;
if ((count < 1) || (count > 11))
return -EINVAL;
@@ -642,11 +652,8 @@ static ssize_t cifs_security_flags_proc_write(struct file *file,
if (count < 3) {
/* single char or single char followed by null */
c = flags_string[0];
- if (c == '0' || c == 'n' || c == 'N') {
- global_secflags = CIFSSEC_DEF; /* default */
- return count;
- } else if (c == '1' || c == 'y' || c == 'Y') {
- global_secflags = CIFSSEC_MAX;
+ if (strtobool(&c, &bv) == 0) {
+ global_secflags = bv ? CIFSSEC_MAX : CIFSSEC_DEF;
return count;
} else if (!isdigit(c)) {
cifs_dbg(VFS, "Invalid SecurityFlags: %s\n",
--
1.9.2

[Alexander Bokovoy]

Post by Andy Shevchenko
---
fs/cifs/cifs_debug.c | 51 +++++++++++++++++++++++++++++----------------------
1 file changed, 29 insertions(+), 22 deletions(-)
diff --git a/fs/cifs/cifs_debug.c b/fs/cifs/cifs_debug.c
index f3ac415..fa78b68 100644
--- a/fs/cifs/cifs_debug.c
+++ b/fs/cifs/cifs_debug.c
@@ -274,6 +274,7 @@ static ssize_t cifs_stats_proc_write(struct file *file,
const char __user *buffer, size_t count, loff_t *ppos)
{
char c;
+ bool bv;
int rc;
struct list_head *tmp1, *tmp2, *tmp3;
struct TCP_Server_Info *server;
@@ -284,7 +285,7 @@ static ssize_t cifs_stats_proc_write(struct file *file,
if (rc)
return rc;
- if (c == '1' || c == 'y' || c == 'Y' || c == '0') {
+ if (strtobool(&c, &bv) == 0) {
#ifdef CONFIG_CIFS_STATS2
atomic_set(&totBufAllocCount, 0);
atomic_set(&totSmBufAllocCount, 0);
@@ -451,15 +452,14 @@ static ssize_t cifsFYI_proc_write(struct file *file, const char __user *buffer,
size_t count, loff_t *ppos)
{
char c;
+ bool bv;
int rc;
rc = get_user(c, buffer);
if (rc)
return rc;
- if (c == '0' || c == 'n' || c == 'N')
- cifsFYI = 0;
- else if (c == '1' || c == 'y' || c == 'Y')
- cifsFYI = 1;
+ if (strtobool(&c, &bv) == 0)
+ cifsFYI = bv;
else if ((c > '1') && (c <= '9'))
cifsFYI = (int) (c - '0'); /* see cifs_debug.h for meanings */
@@ -490,15 +490,18 @@ static ssize_t cifs_linux_ext_proc_write(struct file *file,
const char __user *buffer, size_t count, loff_t *ppos)
{
char c;
+ bool bv;
int rc;
rc = get_user(c, buffer);
if (rc)
return rc;
- if (c == '0' || c == 'n' || c == 'N')
- linuxExtEnabled = 0;
- else if (c == '1' || c == 'y' || c == 'Y')
- linuxExtEnabled = 1;
+
+ rc = strtobool(&c, &bv);
+ if (rc)
+ return rc;
+
+ linuxExtEnabled = bv;
return count;
}

This changes the returned value of cifs_linux_ext_proc_write() to
-EINVAL if strtobool() was not able to find a boolean value.
Previously setting anything non-bool wouldn't cause this side-effect.

Steve, is it OK to you? This is certainly a visible change in that
writing to a proc entry will cause user-space error where it was
ignored before.

Post by Andy Shevchenko
@@ -527,15 +530,18 @@ static ssize_t cifs_lookup_cache_proc_write(struct file *file,
const char __user *buffer, size_t count, loff_t *ppos)
{
char c;
+ bool bv;
int rc;
rc = get_user(c, buffer);
if (rc)
return rc;
- if (c == '0' || c == 'n' || c == 'N')
- lookupCacheEnabled = 0;
- else if (c == '1' || c == 'y' || c == 'Y')
- lookupCacheEnabled = 1;
+
+ rc = strtobool(&c, &bv);
+ if (rc)
+ return rc;
+
+ lookupCacheEnabled = bv;
return count;
}

Same here.

Post by Andy Shevchenko
@@ -564,15 +570,18 @@ static ssize_t traceSMB_proc_write(struct file *file, const char __user *buffer,
size_t count, loff_t *ppos)
{
char c;
+ bool bv;
int rc;
rc = get_user(c, buffer);
if (rc)
return rc;
- if (c == '0' || c == 'n' || c == 'N')
- traceSMB = 0;
- else if (c == '1' || c == 'y' || c == 'Y')
- traceSMB = 1;
+
+ rc = strtobool(&c, &bv);
+ if (rc)
+ return rc;
+
+ traceSMB = bv;
return count;
}

same here.

Post by Andy Shevchenko
@@ -630,6 +639,7 @@ static ssize_t cifs_security_flags_proc_write(struct file *file,
unsigned int flags;
char flags_string[12];
char c;
+ bool bv;
if ((count < 1) || (count > 11))
return -EINVAL;
@@ -642,11 +652,8 @@ static ssize_t cifs_security_flags_proc_write(struct file *file,
if (count < 3) {
/* single char or single char followed by null */
c = flags_string[0];
- if (c == '0' || c == 'n' || c == 'N') {
- global_secflags = CIFSSEC_DEF; /* default */
- return count;
- } else if (c == '1' || c == 'y' || c == 'Y') {
- global_secflags = CIFSSEC_MAX;
+ if (strtobool(&c, &bv) == 0) {
+ global_secflags = bv ? CIFSSEC_MAX : CIFSSEC_DEF;
return count;
} else if (!isdigit(c)) {
cifs_dbg(VFS, "Invalid SecurityFlags: %s\n",
--
1.9.2

[Andy Shevchenko]

Post by Alexander Bokovoy

Post by Andy Shevchenko
---
fs/cifs/cifs_debug.c | 51 +++++++++++++++++++++++++++++----------------------
1 file changed, 29 insertions(+), 22 deletions(-)
diff --git a/fs/cifs/cifs_debug.c b/fs/cifs/cifs_debug.c
index f3ac415..fa78b68 100644
--- a/fs/cifs/cifs_debug.c
+++ b/fs/cifs/cifs_debug.c
@@ -274,6 +274,7 @@ static ssize_t cifs_stats_proc_write(struct file *file,
const char __user *buffer, size_t count, loff_t *ppos)
{
char c;
+ bool bv;
int rc;
struct list_head *tmp1, *tmp2, *tmp3;
struct TCP_Server_Info *server;
@@ -284,7 +285,7 @@ static ssize_t cifs_stats_proc_write(struct file *file,
if (rc)
return rc;
- if (c == '1' || c == 'y' || c == 'Y' || c == '0') {
+ if (strtobool(&c, &bv) == 0) {
#ifdef CONFIG_CIFS_STATS2
atomic_set(&totBufAllocCount, 0);
atomic_set(&totSmBufAllocCount, 0);
@@ -451,15 +452,14 @@ static ssize_t cifsFYI_proc_write(struct file *file, const char __user *buffer,
size_t count, loff_t *ppos)
{
char c;
+ bool bv;
int rc;
rc = get_user(c, buffer);
if (rc)
return rc;
- if (c == '0' || c == 'n' || c == 'N')
- cifsFYI = 0;
- else if (c == '1' || c == 'y' || c == 'Y')
- cifsFYI = 1;
+ if (strtobool(&c, &bv) == 0)
+ cifsFYI = bv;
else if ((c > '1') && (c <= '9'))
cifsFYI = (int) (c - '0'); /* see cifs_debug.h for meanings */
@@ -490,15 +490,18 @@ static ssize_t cifs_linux_ext_proc_write(struct file *file,
const char __user *buffer, size_t count, loff_t *ppos)
{
char c;
+ bool bv;
int rc;
rc = get_user(c, buffer);
if (rc)
return rc;
- if (c == '0' || c == 'n' || c == 'N')
- linuxExtEnabled = 0;
- else if (c == '1' || c == 'y' || c == 'Y')
- linuxExtEnabled = 1;
+
+ rc = strtobool(&c, &bv);
+ if (rc)
+ return rc;
+
+ linuxExtEnabled = bv;
return count;
}

This changes the returned value of cifs_linux_ext_proc_write() to
-EINVAL if strtobool() was not able to find a boolean value.
Previously setting anything non-bool wouldn't cause this side-effect.
Steve, is it OK to you? This is certainly a visible change in that
writing to a proc entry will cause user-space error where it was
ignored before.

Actually, good point. I have to mention this in the commit message.

Post by Alexander Bokovoy

Post by Andy Shevchenko
@@ -527,15 +530,18 @@ static ssize_t cifs_lookup_cache_proc_write(struct file *file,
const char __user *buffer, size_t count, loff_t *ppos)
{
char c;
+ bool bv;
int rc;
rc = get_user(c, buffer);
if (rc)
return rc;
- if (c == '0' || c == 'n' || c == 'N')
- lookupCacheEnabled = 0;
- else if (c == '1' || c == 'y' || c == 'Y')
- lookupCacheEnabled = 1;
+
+ rc = strtobool(&c, &bv);
+ if (rc)
+ return rc;
+
+ lookupCacheEnabled = bv;
return count;
}

Same here.

Post by Andy Shevchenko
@@ -564,15 +570,18 @@ static ssize_t traceSMB_proc_write(struct file *file, const char __user *buffer,
size_t count, loff_t *ppos)
{
char c;
+ bool bv;
int rc;
rc = get_user(c, buffer);
if (rc)
return rc;
- if (c == '0' || c == 'n' || c == 'N')
- traceSMB = 0;
- else if (c == '1' || c == 'y' || c == 'Y')
- traceSMB = 1;
+
+ rc = strtobool(&c, &bv);
+ if (rc)
+ return rc;
+
+ traceSMB = bv;
return count;
}

same here.

Post by Andy Shevchenko
@@ -630,6 +639,7 @@ static ssize_t cifs_security_flags_proc_write(struct file *file,
unsigned int flags;
char flags_string[12];
char c;
+ bool bv;
if ((count < 1) || (count > 11))
return -EINVAL;
@@ -642,11 +652,8 @@ static ssize_t cifs_security_flags_proc_write(struct file *file,
if (count < 3) {
/* single char or single char followed by null */
c = flags_string[0];
- if (c == '0' || c == 'n' || c == 'N') {
- global_secflags = CIFSSEC_DEF; /* default */
- return count;
- } else if (c == '1' || c == 'y' || c == 'Y') {
- global_secflags = CIFSSEC_MAX;
+ if (strtobool(&c, &bv) == 0) {
+ global_secflags = bv ? CIFSSEC_MAX : CIFSSEC_DEF;
return count;
} else if (!isdigit(c)) {
cifs_dbg(VFS, "Invalid SecurityFlags: %s\n",
--
1.9.2

[Jeff Layton]

On Mon, 12 May 2014 07:41:14 +0300

Post by Alexander Bokovoy

Post by Andy Shevchenko
---
fs/cifs/cifs_debug.c | 51 +++++++++++++++++++++++++++++----------------------
1 file changed, 29 insertions(+), 22 deletions(-)
diff --git a/fs/cifs/cifs_debug.c b/fs/cifs/cifs_debug.c
index f3ac415..fa78b68 100644
--- a/fs/cifs/cifs_debug.c
+++ b/fs/cifs/cifs_debug.c
@@ -274,6 +274,7 @@ static ssize_t cifs_stats_proc_write(struct file *file,
const char __user *buffer, size_t count, loff_t *ppos)
{
char c;
+ bool bv;
int rc;
struct list_head *tmp1, *tmp2, *tmp3;
struct TCP_Server_Info *server;
@@ -284,7 +285,7 @@ static ssize_t cifs_stats_proc_write(struct file *file,
if (rc)
return rc;
- if (c == '1' || c == 'y' || c == 'Y' || c == '0') {
+ if (strtobool(&c, &bv) == 0) {
#ifdef CONFIG_CIFS_STATS2
atomic_set(&totBufAllocCount, 0);
atomic_set(&totSmBufAllocCount, 0);
@@ -451,15 +452,14 @@ static ssize_t cifsFYI_proc_write(struct file *file, const char __user *buffer,
size_t count, loff_t *ppos)
{
char c;
+ bool bv;
int rc;
rc = get_user(c, buffer);
if (rc)
return rc;
- if (c == '0' || c == 'n' || c == 'N')
- cifsFYI = 0;
- else if (c == '1' || c == 'y' || c == 'Y')
- cifsFYI = 1;
+ if (strtobool(&c, &bv) == 0)
+ cifsFYI = bv;
else if ((c > '1') && (c <= '9'))
cifsFYI = (int) (c - '0'); /* see cifs_debug.h for meanings */
@@ -490,15 +490,18 @@ static ssize_t cifs_linux_ext_proc_write(struct file *file,
const char __user *buffer, size_t count, loff_t *ppos)
{
char c;
+ bool bv;
int rc;
rc = get_user(c, buffer);
if (rc)
return rc;
- if (c == '0' || c == 'n' || c == 'N')
- linuxExtEnabled = 0;
- else if (c == '1' || c == 'y' || c == 'Y')
- linuxExtEnabled = 1;
+
+ rc = strtobool(&c, &bv);
+ if (rc)
+ return rc;
+
+ linuxExtEnabled = bv;
return count;
}

This changes the returned value of cifs_linux_ext_proc_write() to
-EINVAL if strtobool() was not able to find a boolean value.
Previously setting anything non-bool wouldn't cause this side-effect.
Steve, is it OK to you? This is certainly a visible change in that
writing to a proc entry will cause user-space error where it was
ignored before.

I think returning an error there is the right thing to do. If someone
is writing to this file, they likely mean for it to have some sort of
an effect.

Honestly though, the *best* thing would be to (gradually) convert these
procfile switches into module options. The way it stands now, it's
really hard to set these at boot time. You have to:

# modprobe cifs
# echo N > /proc/fs/cifs/LinuxExtensionsEnabled
# mount -t cifs ...

Making sure that gets done for anything in /etc/fstab is...tricky.

What would be best is to add module options that affect these
variables that could be set via /etc/modprobe.d.

Then, either add a warning printk when someone writes to these files
that they'll soon be deprecated (in 2-3 releases), or maybe turn them
into symlinks that point to the files under /sys/module/cifs/parameters.

Post by Alexander Bokovoy

Post by Andy Shevchenko
@@ -527,15 +530,18 @@ static ssize_t cifs_lookup_cache_proc_write(struct file *file,
const char __user *buffer, size_t count, loff_t *ppos)
{
char c;
+ bool bv;
int rc;
rc = get_user(c, buffer);
if (rc)
return rc;
- if (c == '0' || c == 'n' || c == 'N')
- lookupCacheEnabled = 0;
- else if (c == '1' || c == 'y' || c == 'Y')
- lookupCacheEnabled = 1;
+
+ rc = strtobool(&c, &bv);
+ if (rc)
+ return rc;
+
+ lookupCacheEnabled = bv;
return count;
}

Same here.

Post by Andy Shevchenko
@@ -564,15 +570,18 @@ static ssize_t traceSMB_proc_write(struct file *file, const char __user *buffer,
size_t count, loff_t *ppos)
{
char c;
+ bool bv;
int rc;
rc = get_user(c, buffer);
if (rc)
return rc;
- if (c == '0' || c == 'n' || c == 'N')
- traceSMB = 0;
- else if (c == '1' || c == 'y' || c == 'Y')
- traceSMB = 1;
+
+ rc = strtobool(&c, &bv);
+ if (rc)
+ return rc;
+
+ traceSMB = bv;
return count;
}

same here.

Post by Andy Shevchenko
@@ -630,6 +639,7 @@ static ssize_t cifs_security_flags_proc_write(struct file *file,
unsigned int flags;
char flags_string[12];
char c;
+ bool bv;
if ((count < 1) || (count > 11))
return -EINVAL;
@@ -642,11 +652,8 @@ static ssize_t cifs_security_flags_proc_write(struct file *file,
if (count < 3) {
/* single char or single char followed by null */
c = flags_string[0];
- if (c == '0' || c == 'n' || c == 'N') {
- global_secflags = CIFSSEC_DEF; /* default */
- return count;
- } else if (c == '1' || c == 'y' || c == 'Y') {
- global_secflags = CIFSSEC_MAX;
+ if (strtobool(&c, &bv) == 0) {
+ global_secflags = bv ? CIFSSEC_MAX : CIFSSEC_DEF;
return count;
} else if (!isdigit(c)) {
cifs_dbg(VFS, "Invalid SecurityFlags: %s\n",
--
1.9.2

[Alexander Bokovoy]

Post by Jeff Layton

Post by Alexander Bokovoy
This changes the returned value of cifs_linux_ext_proc_write() to
-EINVAL if strtobool() was not able to find a boolean value.
Previously setting anything non-bool wouldn't cause this side-effect.
Steve, is it OK to you? This is certainly a visible change in that
writing to a proc entry will cause user-space error where it was
ignored before.

I think returning an error there is the right thing to do. If someone
is writing to this file, they likely mean for it to have some sort of
an effect.
Honestly though, the *best* thing would be to (gradually) convert these
procfile switches into module options. The way it stands now, it's
# modprobe cifs
# echo N > /proc/fs/cifs/LinuxExtensionsEnabled
# mount -t cifs ...
Making sure that gets done for anything in /etc/fstab is...tricky.
What would be best is to add module options that affect these
variables that could be set via /etc/modprobe.d.
Then, either add a warning printk when someone writes to these files
that they'll soon be deprecated (in 2-3 releases), or maybe turn them
into symlinks that point to the files under /sys/module/cifs/parameters.

I agree.

I think strtobool() patches are fine (+ change to the commit message).
The change to module options should certainly be done in a separate
patchset.

[Andy Shevchenko]

Post by Alexander Bokovoy

Post by Jeff Layton

Post by Alexander Bokovoy
This changes the returned value of cifs_linux_ext_proc_write() to
-EINVAL if strtobool() was not able to find a boolean value.
Previously setting anything non-bool wouldn't cause this side-effect.
Steve, is it OK to you? This is certainly a visible change in that
writing to a proc entry will cause user-space error where it was
ignored before.

I think returning an error there is the right thing to do. If someone
is writing to this file, they likely mean for it to have some sort of
an effect.
Honestly though, the *best* thing would be to (gradually) convert these
procfile switches into module options. The way it stands now, it's
# modprobe cifs
# echo N > /proc/fs/cifs/LinuxExtensionsEnabled
# mount -t cifs ...
Making sure that gets done for anything in /etc/fstab is...tricky.
What would be best is to add module options that affect these
variables that could be set via /etc/modprobe.d.
Then, either add a warning printk when someone writes to these files
that they'll soon be deprecated (in 2-3 releases), or maybe turn them
into symlinks that point to the files under /sys/module/cifs/parameters.

I agree.
I think strtobool() patches are fine (+ change to the commit message).

I will resend v2 soon. Could I add your Acked-by or Reviewed-by?

Post by Alexander Bokovoy
The change to module options should certainly be done in a separate
patchset.

[Jeff Layton]

On Mon, 12 May 2014 16:59:55 +0300

Post by Andy Shevchenko

Post by Alexander Bokovoy

Post by Jeff Layton

Post by Alexander Bokovoy
This changes the returned value of cifs_linux_ext_proc_write() to
-EINVAL if strtobool() was not able to find a boolean value.
Previously setting anything non-bool wouldn't cause this side-effect.
Steve, is it OK to you? This is certainly a visible change in that
writing to a proc entry will cause user-space error where it was
ignored before.

I think returning an error there is the right thing to do. If someone
is writing to this file, they likely mean for it to have some sort of
an effect.
Honestly though, the *best* thing would be to (gradually) convert these
procfile switches into module options. The way it stands now, it's
# modprobe cifs
# echo N > /proc/fs/cifs/LinuxExtensionsEnabled
# mount -t cifs ...
Making sure that gets done for anything in /etc/fstab is...tricky.
What would be best is to add module options that affect these
variables that could be set via /etc/modprobe.d.
Then, either add a warning printk when someone writes to these files
that they'll soon be deprecated (in 2-3 releases), or maybe turn them
into symlinks that point to the files under /sys/module/cifs/parameters.

I agree.
I think strtobool() patches are fine (+ change to the commit message).

I will resend v2 soon. Could I add your Acked-by or Reviewed-by?

Sure, you can add mine...

Post by Andy Shevchenko

Post by Alexander Bokovoy
The change to module options should certainly be done in a separate
patchset.

Agreed.

-- Jeff

[Alexander Bokovoy]

Post by Andy Shevchenko

Post by Alexander Bokovoy

Post by Jeff Layton

Post by Alexander Bokovoy
This changes the returned value of cifs_linux_ext_proc_write() to
-EINVAL if strtobool() was not able to find a boolean value.
Previously setting anything non-bool wouldn't cause this side-effect.
Steve, is it OK to you? This is certainly a visible change in that
writing to a proc entry will cause user-space error where it was
ignored before.

I think returning an error there is the right thing to do. If someone
is writing to this file, they likely mean for it to have some sort of
an effect.
Honestly though, the *best* thing would be to (gradually) convert these
procfile switches into module options. The way it stands now, it's
# modprobe cifs
# echo N > /proc/fs/cifs/LinuxExtensionsEnabled
# mount -t cifs ...
Making sure that gets done for anything in /etc/fstab is...tricky.
What would be best is to add module options that affect these
variables that could be set via /etc/modprobe.d.
Then, either add a warning printk when someone writes to these files
that they'll soon be deprecated (in 2-3 releases), or maybe turn them
into symlinks that point to the files under /sys/module/cifs/parameters.

I agree.
I think strtobool() patches are fine (+ change to the commit message).

I will resend v2 soon. Could I add your Acked-by or Reviewed-by?

Sure, you can add mine.