* [PATCH 0/2] staging: lustre: lprocfs: Fix coding style issues
@ 2017-05-04 16:13 ` Mathias Rav
0 siblings, 0 replies; 14+ messages in thread
From: Mathias Rav @ 2017-05-04 16:13 UTC (permalink / raw)
To: Greg Kroah-Hartman, devel, Oleg Drokin, Andreas Dilger, James Simmons
Cc: Linux Kernel Mailing List, Lustre Development List
This patchset fixes two style issues in lprocfs_status.c related to
simple_strtoul and seq_printf (reported by checkpatch).
There's a slight change in lustre debugfs write semantics: Using kstrtox causes
EINVAL when the written number is followed by other (garbage) characters,
whereas previously the garbage would be ignored and such a write would succeed.
Mathias Rav (2):
staging: lustre: lprocfs: Use kstrtouint_from_user
staging: lustre: lprocfs: Use seq_puts
.../lustre/lustre/obdclass/lprocfs_status.c | 42 ++++++---------------
1 file changed, 11 insertions(+), 31 deletions(-)
^ permalink raw reply [flat|nested] 14+ messages in thread
* [lustre-devel] [PATCH 0/2] staging: lustre: lprocfs: Fix coding style issues
@ 2017-05-04 16:13 ` Mathias Rav
0 siblings, 0 replies; 14+ messages in thread
From: Mathias Rav @ 2017-05-04 16:13 UTC (permalink / raw)
To: Greg Kroah-Hartman, devel, Oleg Drokin, Andreas Dilger, James Simmons
Cc: Linux Kernel Mailing List, Lustre Development List
This patchset fixes two style issues in lprocfs_status.c related to
simple_strtoul and seq_printf (reported by checkpatch).
There's a slight change in lustre debugfs write semantics: Using kstrtox causes
EINVAL when the written number is followed by other (garbage) characters,
whereas previously the garbage would be ignored and such a write would succeed.
Mathias Rav (2):
staging: lustre: lprocfs: Use kstrtouint_from_user
staging: lustre: lprocfs: Use seq_puts
.../lustre/lustre/obdclass/lprocfs_status.c | 42 ++++++---------------
1 file changed, 11 insertions(+), 31 deletions(-)
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/2] staging: lustre: lprocfs: Use kstrtouint_from_user
2017-05-04 16:13 ` [lustre-devel] " Mathias Rav
@ 2017-05-04 16:13 ` Mathias Rav
-1 siblings, 0 replies; 14+ messages in thread
From: Mathias Rav @ 2017-05-04 16:13 UTC (permalink / raw)
To: Greg Kroah-Hartman, devel, Oleg Drokin, Andreas Dilger, James Simmons
Cc: Linux Kernel Mailing List, Lustre Development List, Mathias Rav
Prefer kstrtouint_from_user to copy_from_user+simple_strtoul.
The helper function lprocfs_wr_uint() is only used to implement
"dump_granted_max" in debugfs.
Note the slight change in semantics: The previous implementation using
simple_strtoul allows garbage after the number, whereas kstrtox only allows
a trailing line break. The previous implementation allowed a write of zero
bytes whereas kstrtox will return -EINVAL. Since this only affects a single
debugfs endpoint, this should be a permissible slight change of semantics
in exchange for 18 fewer lines of code.
Signed-off-by: Mathias Rav <mathiasrav@gmail.com>
---
.../lustre/lustre/obdclass/lprocfs_status.c | 22 +---------------------
1 file changed, 1 insertion(+), 21 deletions(-)
diff --git a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
index 1ec6e3767d81..338ce34d6514 100644
--- a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
+++ b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
@@ -399,27 +399,7 @@ EXPORT_SYMBOL(lprocfs_rd_uint);
int lprocfs_wr_uint(struct file *file, const char __user *buffer,
unsigned long count, void *data)
{
- unsigned *p = data;
- char dummy[MAX_STRING_SIZE + 1], *end;
- unsigned long tmp;
-
- if (count >= sizeof(dummy))
- return -EINVAL;
-
- if (count == 0)
- return 0;
-
- if (copy_from_user(dummy, buffer, count))
- return -EFAULT;
-
- dummy[count] = '\0';
-
- tmp = simple_strtoul(dummy, &end, 0);
- if (dummy == end)
- return -EINVAL;
-
- *p = (unsigned int)tmp;
- return count;
+ return kstrtouint_from_user(buffer, count, 0, (unsigned int *)data);
}
EXPORT_SYMBOL(lprocfs_wr_uint);
--
2.12.2
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [lustre-devel] [PATCH 1/2] staging: lustre: lprocfs: Use kstrtouint_from_user
@ 2017-05-04 16:13 ` Mathias Rav
0 siblings, 0 replies; 14+ messages in thread
From: Mathias Rav @ 2017-05-04 16:13 UTC (permalink / raw)
To: Greg Kroah-Hartman, devel, Oleg Drokin, Andreas Dilger, James Simmons
Cc: Linux Kernel Mailing List, Lustre Development List, Mathias Rav
Prefer kstrtouint_from_user to copy_from_user+simple_strtoul.
The helper function lprocfs_wr_uint() is only used to implement
"dump_granted_max" in debugfs.
Note the slight change in semantics: The previous implementation using
simple_strtoul allows garbage after the number, whereas kstrtox only allows
a trailing line break. The previous implementation allowed a write of zero
bytes whereas kstrtox will return -EINVAL. Since this only affects a single
debugfs endpoint, this should be a permissible slight change of semantics
in exchange for 18 fewer lines of code.
Signed-off-by: Mathias Rav <mathiasrav@gmail.com>
---
.../lustre/lustre/obdclass/lprocfs_status.c | 22 +---------------------
1 file changed, 1 insertion(+), 21 deletions(-)
diff --git a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
index 1ec6e3767d81..338ce34d6514 100644
--- a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
+++ b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
@@ -399,27 +399,7 @@ EXPORT_SYMBOL(lprocfs_rd_uint);
int lprocfs_wr_uint(struct file *file, const char __user *buffer,
unsigned long count, void *data)
{
- unsigned *p = data;
- char dummy[MAX_STRING_SIZE + 1], *end;
- unsigned long tmp;
-
- if (count >= sizeof(dummy))
- return -EINVAL;
-
- if (count == 0)
- return 0;
-
- if (copy_from_user(dummy, buffer, count))
- return -EFAULT;
-
- dummy[count] = '\0';
-
- tmp = simple_strtoul(dummy, &end, 0);
- if (dummy == end)
- return -EINVAL;
-
- *p = (unsigned int)tmp;
- return count;
+ return kstrtouint_from_user(buffer, count, 0, (unsigned int *)data);
}
EXPORT_SYMBOL(lprocfs_wr_uint);
--
2.12.2
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/2] staging: lustre: lprocfs: Use seq_puts
2017-05-04 16:13 ` [lustre-devel] " Mathias Rav
@ 2017-05-04 16:13 ` Mathias Rav
-1 siblings, 0 replies; 14+ messages in thread
From: Mathias Rav @ 2017-05-04 16:13 UTC (permalink / raw)
To: Greg Kroah-Hartman, devel, Oleg Drokin, Andreas Dilger, James Simmons
Cc: Linux Kernel Mailing List, Lustre Development List, Mathias Rav
Replace all occurrences of seq_printf with no formatting directives
in lprocfs_status.c with seq_puts.
Reported by checkpatch.pl: "WARNING: Prefer seq_puts to seq_printf".
Signed-off-by: Mathias Rav <mathiasrav@gmail.com>
---
.../staging/lustre/lustre/obdclass/lprocfs_status.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
index 338ce34d6514..b5e0e46777ea 100644
--- a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
+++ b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
@@ -716,7 +716,7 @@ static int obd_import_flags2str(struct obd_import *imp, struct seq_file *m)
bool first = true;
if (imp->imp_obd->obd_no_recov) {
- seq_printf(m, "no_recov");
+ seq_puts(m, "no_recov");
first = false;
}
@@ -782,15 +782,15 @@ int lprocfs_rd_import(struct seq_file *m, void *data)
imp->imp_connect_data.ocd_instance);
obd_connect_seq_flags2str(m, imp->imp_connect_data.ocd_connect_flags,
", ");
- seq_printf(m, " ]\n");
+ seq_puts(m, " ]\n");
obd_connect_data_seqprint(m, ocd);
- seq_printf(m, " import_flags: [ ");
+ seq_puts(m, " import_flags: [ ");
obd_import_flags2str(imp, m);
- seq_printf(m,
- " ]\n"
- " connection:\n"
- " failover_nids: [ ");
+ seq_puts(m,
+ " ]\n"
+ " connection:\n"
+ " failover_nids: [ ");
spin_lock(&imp->imp_lock);
j = 0;
list_for_each_entry(conn, &imp->imp_conn_list, oic_item) {
@@ -923,7 +923,7 @@ int lprocfs_rd_state(struct seq_file *m, void *data)
seq_printf(m, "current_state: %s\n",
ptlrpc_import_state_name(imp->imp_state));
- seq_printf(m, "state_history:\n");
+ seq_puts(m, "state_history:\n");
k = imp->imp_state_hist_idx;
for (j = 0; j < IMP_STATE_HIST_LEN; j++) {
struct import_state_hist *ish =
@@ -945,7 +945,7 @@ int lprocfs_at_hist_helper(struct seq_file *m, struct adaptive_timeout *at)
for (i = 0; i < AT_BINS; i++)
seq_printf(m, "%3u ", at->at_hist[i]);
- seq_printf(m, "\n");
+ seq_puts(m, "\n");
return 0;
}
EXPORT_SYMBOL(lprocfs_at_hist_helper);
@@ -1013,7 +1013,7 @@ int lprocfs_rd_connect_flags(struct seq_file *m, void *data)
flags = obd->u.cli.cl_import->imp_connect_data.ocd_connect_flags;
seq_printf(m, "flags=%#llx\n", flags);
obd_connect_seq_flags2str(m, flags, "\n");
- seq_printf(m, "\n");
+ seq_puts(m, "\n");
up_read(&obd->u.cli.cl_sem);
return 0;
}
--
2.12.2
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [lustre-devel] [PATCH 2/2] staging: lustre: lprocfs: Use seq_puts
@ 2017-05-04 16:13 ` Mathias Rav
0 siblings, 0 replies; 14+ messages in thread
From: Mathias Rav @ 2017-05-04 16:13 UTC (permalink / raw)
To: Greg Kroah-Hartman, devel, Oleg Drokin, Andreas Dilger, James Simmons
Cc: Linux Kernel Mailing List, Lustre Development List, Mathias Rav
Replace all occurrences of seq_printf with no formatting directives
in lprocfs_status.c with seq_puts.
Reported by checkpatch.pl: "WARNING: Prefer seq_puts to seq_printf".
Signed-off-by: Mathias Rav <mathiasrav@gmail.com>
---
.../staging/lustre/lustre/obdclass/lprocfs_status.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
index 338ce34d6514..b5e0e46777ea 100644
--- a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
+++ b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
@@ -716,7 +716,7 @@ static int obd_import_flags2str(struct obd_import *imp, struct seq_file *m)
bool first = true;
if (imp->imp_obd->obd_no_recov) {
- seq_printf(m, "no_recov");
+ seq_puts(m, "no_recov");
first = false;
}
@@ -782,15 +782,15 @@ int lprocfs_rd_import(struct seq_file *m, void *data)
imp->imp_connect_data.ocd_instance);
obd_connect_seq_flags2str(m, imp->imp_connect_data.ocd_connect_flags,
", ");
- seq_printf(m, " ]\n");
+ seq_puts(m, " ]\n");
obd_connect_data_seqprint(m, ocd);
- seq_printf(m, " import_flags: [ ");
+ seq_puts(m, " import_flags: [ ");
obd_import_flags2str(imp, m);
- seq_printf(m,
- " ]\n"
- " connection:\n"
- " failover_nids: [ ");
+ seq_puts(m,
+ " ]\n"
+ " connection:\n"
+ " failover_nids: [ ");
spin_lock(&imp->imp_lock);
j = 0;
list_for_each_entry(conn, &imp->imp_conn_list, oic_item) {
@@ -923,7 +923,7 @@ int lprocfs_rd_state(struct seq_file *m, void *data)
seq_printf(m, "current_state: %s\n",
ptlrpc_import_state_name(imp->imp_state));
- seq_printf(m, "state_history:\n");
+ seq_puts(m, "state_history:\n");
k = imp->imp_state_hist_idx;
for (j = 0; j < IMP_STATE_HIST_LEN; j++) {
struct import_state_hist *ish =
@@ -945,7 +945,7 @@ int lprocfs_at_hist_helper(struct seq_file *m, struct adaptive_timeout *at)
for (i = 0; i < AT_BINS; i++)
seq_printf(m, "%3u ", at->at_hist[i]);
- seq_printf(m, "\n");
+ seq_puts(m, "\n");
return 0;
}
EXPORT_SYMBOL(lprocfs_at_hist_helper);
@@ -1013,7 +1013,7 @@ int lprocfs_rd_connect_flags(struct seq_file *m, void *data)
flags = obd->u.cli.cl_import->imp_connect_data.ocd_connect_flags;
seq_printf(m, "flags=%#llx\n", flags);
obd_connect_seq_flags2str(m, flags, "\n");
- seq_printf(m, "\n");
+ seq_puts(m, "\n");
up_read(&obd->u.cli.cl_sem);
return 0;
}
--
2.12.2
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] staging: lustre: lprocfs: Use kstrtouint_from_user
2017-05-04 16:13 ` [lustre-devel] " Mathias Rav
@ 2017-05-18 13:53 ` Greg Kroah-Hartman
-1 siblings, 0 replies; 14+ messages in thread
From: Greg Kroah-Hartman @ 2017-05-18 13:53 UTC (permalink / raw)
To: Mathias Rav
Cc: devel, Oleg Drokin, Andreas Dilger, James Simmons,
Linux Kernel Mailing List, Lustre Development List
On Thu, May 04, 2017 at 12:13:38PM -0400, Mathias Rav wrote:
> Prefer kstrtouint_from_user to copy_from_user+simple_strtoul.
>
> The helper function lprocfs_wr_uint() is only used to implement
> "dump_granted_max" in debugfs.
>
> Note the slight change in semantics: The previous implementation using
> simple_strtoul allows garbage after the number, whereas kstrtox only allows
> a trailing line break. The previous implementation allowed a write of zero
> bytes whereas kstrtox will return -EINVAL. Since this only affects a single
> debugfs endpoint, this should be a permissible slight change of semantics
> in exchange for 18 fewer lines of code.
>
> Signed-off-by: Mathias Rav <mathiasrav@gmail.com>
> ---
> .../lustre/lustre/obdclass/lprocfs_status.c | 22 +---------------------
> 1 file changed, 1 insertion(+), 21 deletions(-)
>
> diff --git a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
> index 1ec6e3767d81..338ce34d6514 100644
> --- a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
> +++ b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
> @@ -399,27 +399,7 @@ EXPORT_SYMBOL(lprocfs_rd_uint);
> int lprocfs_wr_uint(struct file *file, const char __user *buffer,
> unsigned long count, void *data)
> {
> - unsigned *p = data;
> - char dummy[MAX_STRING_SIZE + 1], *end;
> - unsigned long tmp;
> -
> - if (count >= sizeof(dummy))
> - return -EINVAL;
> -
> - if (count == 0)
> - return 0;
> -
> - if (copy_from_user(dummy, buffer, count))
> - return -EFAULT;
> -
> - dummy[count] = '\0';
> -
> - tmp = simple_strtoul(dummy, &end, 0);
> - if (dummy == end)
> - return -EINVAL;
> -
> - *p = (unsigned int)tmp;
> - return count;
> + return kstrtouint_from_user(buffer, count, 0, (unsigned int *)data);
Why not just delete this whole function and have the callers make this
call instead? No need for unneeded wrapper functions of core kernel
calls.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 14+ messages in thread
* [lustre-devel] [PATCH 1/2] staging: lustre: lprocfs: Use kstrtouint_from_user
@ 2017-05-18 13:53 ` Greg Kroah-Hartman
0 siblings, 0 replies; 14+ messages in thread
From: Greg Kroah-Hartman @ 2017-05-18 13:53 UTC (permalink / raw)
To: Mathias Rav
Cc: devel, Oleg Drokin, Andreas Dilger, James Simmons,
Linux Kernel Mailing List, Lustre Development List
On Thu, May 04, 2017 at 12:13:38PM -0400, Mathias Rav wrote:
> Prefer kstrtouint_from_user to copy_from_user+simple_strtoul.
>
> The helper function lprocfs_wr_uint() is only used to implement
> "dump_granted_max" in debugfs.
>
> Note the slight change in semantics: The previous implementation using
> simple_strtoul allows garbage after the number, whereas kstrtox only allows
> a trailing line break. The previous implementation allowed a write of zero
> bytes whereas kstrtox will return -EINVAL. Since this only affects a single
> debugfs endpoint, this should be a permissible slight change of semantics
> in exchange for 18 fewer lines of code.
>
> Signed-off-by: Mathias Rav <mathiasrav@gmail.com>
> ---
> .../lustre/lustre/obdclass/lprocfs_status.c | 22 +---------------------
> 1 file changed, 1 insertion(+), 21 deletions(-)
>
> diff --git a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
> index 1ec6e3767d81..338ce34d6514 100644
> --- a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
> +++ b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
> @@ -399,27 +399,7 @@ EXPORT_SYMBOL(lprocfs_rd_uint);
> int lprocfs_wr_uint(struct file *file, const char __user *buffer,
> unsigned long count, void *data)
> {
> - unsigned *p = data;
> - char dummy[MAX_STRING_SIZE + 1], *end;
> - unsigned long tmp;
> -
> - if (count >= sizeof(dummy))
> - return -EINVAL;
> -
> - if (count == 0)
> - return 0;
> -
> - if (copy_from_user(dummy, buffer, count))
> - return -EFAULT;
> -
> - dummy[count] = '\0';
> -
> - tmp = simple_strtoul(dummy, &end, 0);
> - if (dummy == end)
> - return -EINVAL;
> -
> - *p = (unsigned int)tmp;
> - return count;
> + return kstrtouint_from_user(buffer, count, 0, (unsigned int *)data);
Why not just delete this whole function and have the callers make this
call instead? No need for unneeded wrapper functions of core kernel
calls.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] staging: lustre: lprocfs: Use kstrtouint_from_user
2017-05-18 13:53 ` [lustre-devel] " Greg Kroah-Hartman
@ 2017-05-18 14:48 ` Dilger, Andreas
-1 siblings, 0 replies; 14+ messages in thread
From: Dilger, Andreas @ 2017-05-18 14:48 UTC (permalink / raw)
To: Greg Kroah-Hartman, Mathias Rav
Cc: devel, Drokin, Oleg, James Simmons, Linux Kernel Mailing List,
Lustre Development List
On May 18, 2017, at 15:53, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
>
> On Thu, May 04, 2017 at 12:13:38PM -0400, Mathias Rav wrote:
>> Prefer kstrtouint_from_user to copy_from_user+simple_strtoul.
>>
>> The helper function lprocfs_wr_uint() is only used to implement
>> "dump_granted_max" in debugfs.
>>
>> Note the slight change in semantics: The previous implementation using
>> simple_strtoul allows garbage after the number, whereas kstrtox only allows
>> a trailing line break. The previous implementation allowed a write of zero
>> bytes whereas kstrtox will return -EINVAL. Since this only affects a single
>> debugfs endpoint, this should be a permissible slight change of semantics
>> in exchange for 18 fewer lines of code.
>>
>> Signed-off-by: Mathias Rav <mathiasrav@gmail.com>
>> ---
>> .../lustre/lustre/obdclass/lprocfs_status.c | 22 +---------------------
>> 1 file changed, 1 insertion(+), 21 deletions(-)
>>
>> diff --git a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
>> index 1ec6e3767d81..338ce34d6514 100644
>> --- a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
>> +++ b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
>> @@ -399,27 +399,7 @@ EXPORT_SYMBOL(lprocfs_rd_uint);
>> int lprocfs_wr_uint(struct file *file, const char __user *buffer,
>> unsigned long count, void *data)
>> {
>> - unsigned *p = data;
>> - char dummy[MAX_STRING_SIZE + 1], *end;
>> - unsigned long tmp;
>> -
>> - if (count >= sizeof(dummy))
>> - return -EINVAL;
>> -
>> - if (count == 0)
>> - return 0;
>> -
>> - if (copy_from_user(dummy, buffer, count))
>> - return -EFAULT;
>> -
>> - dummy[count] = '\0';
>> -
>> - tmp = simple_strtoul(dummy, &end, 0);
>> - if (dummy == end)
>> - return -EINVAL;
>> -
>> - *p = (unsigned int)tmp;
>> - return count;
>> + return kstrtouint_from_user(buffer, count, 0, (unsigned int *)data);
>
> Why not just delete this whole function and have the callers make this
> call instead? No need for unneeded wrapper functions of core kernel
> calls.
Even better, it looks like this function has no callers on the client and could just
be deleted entirely.
Cheers, Andreas
--
Andreas Dilger
Lustre Principal Architect
Intel Corporation
^ permalink raw reply [flat|nested] 14+ messages in thread
* [lustre-devel] [PATCH 1/2] staging: lustre: lprocfs: Use kstrtouint_from_user
@ 2017-05-18 14:48 ` Dilger, Andreas
0 siblings, 0 replies; 14+ messages in thread
From: Dilger, Andreas @ 2017-05-18 14:48 UTC (permalink / raw)
To: Greg Kroah-Hartman, Mathias Rav
Cc: devel, Drokin, Oleg, James Simmons, Linux Kernel Mailing List,
Lustre Development List
On May 18, 2017, at 15:53, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
>
> On Thu, May 04, 2017 at 12:13:38PM -0400, Mathias Rav wrote:
>> Prefer kstrtouint_from_user to copy_from_user+simple_strtoul.
>>
>> The helper function lprocfs_wr_uint() is only used to implement
>> "dump_granted_max" in debugfs.
>>
>> Note the slight change in semantics: The previous implementation using
>> simple_strtoul allows garbage after the number, whereas kstrtox only allows
>> a trailing line break. The previous implementation allowed a write of zero
>> bytes whereas kstrtox will return -EINVAL. Since this only affects a single
>> debugfs endpoint, this should be a permissible slight change of semantics
>> in exchange for 18 fewer lines of code.
>>
>> Signed-off-by: Mathias Rav <mathiasrav@gmail.com>
>> ---
>> .../lustre/lustre/obdclass/lprocfs_status.c | 22 +---------------------
>> 1 file changed, 1 insertion(+), 21 deletions(-)
>>
>> diff --git a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
>> index 1ec6e3767d81..338ce34d6514 100644
>> --- a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
>> +++ b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
>> @@ -399,27 +399,7 @@ EXPORT_SYMBOL(lprocfs_rd_uint);
>> int lprocfs_wr_uint(struct file *file, const char __user *buffer,
>> unsigned long count, void *data)
>> {
>> - unsigned *p = data;
>> - char dummy[MAX_STRING_SIZE + 1], *end;
>> - unsigned long tmp;
>> -
>> - if (count >= sizeof(dummy))
>> - return -EINVAL;
>> -
>> - if (count == 0)
>> - return 0;
>> -
>> - if (copy_from_user(dummy, buffer, count))
>> - return -EFAULT;
>> -
>> - dummy[count] = '\0';
>> -
>> - tmp = simple_strtoul(dummy, &end, 0);
>> - if (dummy == end)
>> - return -EINVAL;
>> -
>> - *p = (unsigned int)tmp;
>> - return count;
>> + return kstrtouint_from_user(buffer, count, 0, (unsigned int *)data);
>
> Why not just delete this whole function and have the callers make this
> call instead? No need for unneeded wrapper functions of core kernel
> calls.
Even better, it looks like this function has no callers on the client and could just
be deleted entirely.
Cheers, Andreas
--
Andreas Dilger
Lustre Principal Architect
Intel Corporation
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] staging: lustre: lprocfs: Use kstrtouint_from_user
2017-05-18 14:48 ` [lustre-devel] " Dilger, Andreas
@ 2017-05-18 15:13 ` Mathias Rav
-1 siblings, 0 replies; 14+ messages in thread
From: Mathias Rav @ 2017-05-18 15:13 UTC (permalink / raw)
To: Dilger, Andreas, Greg Kroah-Hartman
Cc: devel, Drokin, Oleg, James Simmons, Linux Kernel Mailing List,
Lustre Development List
On Thu, 18 May 2017 14:48:25 +0000
"Dilger, Andreas" <andreas.dilger@intel.com> wrote:
> On May 18, 2017, at 15:53, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> >
> > On Thu, May 04, 2017 at 12:13:38PM -0400, Mathias Rav wrote:
> >> Prefer kstrtouint_from_user to copy_from_user+simple_strtoul.
> >>
> >> The helper function lprocfs_wr_uint() is only used to implement
> >> "dump_granted_max" in debugfs.
> >>
> >> Note the slight change in semantics: The previous implementation using
> >> simple_strtoul allows garbage after the number, whereas kstrtox only allows
> >> a trailing line break. The previous implementation allowed a write of zero
> >> bytes whereas kstrtox will return -EINVAL. Since this only affects a single
> >> debugfs endpoint, this should be a permissible slight change of semantics
> >> in exchange for 18 fewer lines of code.
> >>
> >> Signed-off-by: Mathias Rav <mathiasrav@gmail.com>
> >> ---
> >> .../lustre/lustre/obdclass/lprocfs_status.c | 22 +---------------------
> >> 1 file changed, 1 insertion(+), 21 deletions(-)
> >>
> >> diff --git a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
> >> index 1ec6e3767d81..338ce34d6514 100644
> >> --- a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
> >> +++ b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
> >> @@ -399,27 +399,7 @@ EXPORT_SYMBOL(lprocfs_rd_uint);
> >> int lprocfs_wr_uint(struct file *file, const char __user *buffer,
> >> unsigned long count, void *data)
> >> {
> >> - unsigned *p = data;
> >> - char dummy[MAX_STRING_SIZE + 1], *end;
> >> - unsigned long tmp;
> >> -
> >> - if (count >= sizeof(dummy))
> >> - return -EINVAL;
> >> -
> >> - if (count == 0)
> >> - return 0;
> >> -
> >> - if (copy_from_user(dummy, buffer, count))
> >> - return -EFAULT;
> >> -
> >> - dummy[count] = '\0';
> >> -
> >> - tmp = simple_strtoul(dummy, &end, 0);
> >> - if (dummy == end)
> >> - return -EINVAL;
> >> -
> >> - *p = (unsigned int)tmp;
> >> - return count;
> >> + return kstrtouint_from_user(buffer, count, 0, (unsigned int *)data);
> >
> > Why not just delete this whole function and have the callers make this
> > call instead? No need for unneeded wrapper functions of core kernel
> > calls.
>
> Even better, it looks like this function has no callers on the client and could just
> be deleted entirely.
No, the functions lprocfs_{rd,wr}_uint are used once through a macro:
ldlm/ldlm_resource.c calls LPROC_SEQ_FOPS_RW_TYPE(ldlm_rw, uint)
to define ldlm_rw_uint_seq_{show,write}, which then calls
lprocfs_{rd,wr}_uint which in turn call seq_printf/kstrtouint_from_user.
It seems like much indirection for almost no gain besides hiding
access to ((seq_file *) file->private_data)->private in a macro.
If you agree, I will prepare a patch that switches ldlm_resource to
using the LPROC_SEQ_FOPS macro directly, which allows us to remove two
trivial wrappers.
/Mathias
^ permalink raw reply [flat|nested] 14+ messages in thread
* [lustre-devel] [PATCH 1/2] staging: lustre: lprocfs: Use kstrtouint_from_user
@ 2017-05-18 15:13 ` Mathias Rav
0 siblings, 0 replies; 14+ messages in thread
From: Mathias Rav @ 2017-05-18 15:13 UTC (permalink / raw)
To: Dilger, Andreas, Greg Kroah-Hartman
Cc: devel, Drokin, Oleg, James Simmons, Linux Kernel Mailing List,
Lustre Development List
On Thu, 18 May 2017 14:48:25 +0000
"Dilger, Andreas" <andreas.dilger@intel.com> wrote:
> On May 18, 2017, at 15:53, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> >
> > On Thu, May 04, 2017 at 12:13:38PM -0400, Mathias Rav wrote:
> >> Prefer kstrtouint_from_user to copy_from_user+simple_strtoul.
> >>
> >> The helper function lprocfs_wr_uint() is only used to implement
> >> "dump_granted_max" in debugfs.
> >>
> >> Note the slight change in semantics: The previous implementation using
> >> simple_strtoul allows garbage after the number, whereas kstrtox only allows
> >> a trailing line break. The previous implementation allowed a write of zero
> >> bytes whereas kstrtox will return -EINVAL. Since this only affects a single
> >> debugfs endpoint, this should be a permissible slight change of semantics
> >> in exchange for 18 fewer lines of code.
> >>
> >> Signed-off-by: Mathias Rav <mathiasrav@gmail.com>
> >> ---
> >> .../lustre/lustre/obdclass/lprocfs_status.c | 22 +---------------------
> >> 1 file changed, 1 insertion(+), 21 deletions(-)
> >>
> >> diff --git a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
> >> index 1ec6e3767d81..338ce34d6514 100644
> >> --- a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
> >> +++ b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
> >> @@ -399,27 +399,7 @@ EXPORT_SYMBOL(lprocfs_rd_uint);
> >> int lprocfs_wr_uint(struct file *file, const char __user *buffer,
> >> unsigned long count, void *data)
> >> {
> >> - unsigned *p = data;
> >> - char dummy[MAX_STRING_SIZE + 1], *end;
> >> - unsigned long tmp;
> >> -
> >> - if (count >= sizeof(dummy))
> >> - return -EINVAL;
> >> -
> >> - if (count == 0)
> >> - return 0;
> >> -
> >> - if (copy_from_user(dummy, buffer, count))
> >> - return -EFAULT;
> >> -
> >> - dummy[count] = '\0';
> >> -
> >> - tmp = simple_strtoul(dummy, &end, 0);
> >> - if (dummy == end)
> >> - return -EINVAL;
> >> -
> >> - *p = (unsigned int)tmp;
> >> - return count;
> >> + return kstrtouint_from_user(buffer, count, 0, (unsigned int *)data);
> >
> > Why not just delete this whole function and have the callers make this
> > call instead? No need for unneeded wrapper functions of core kernel
> > calls.
>
> Even better, it looks like this function has no callers on the client and could just
> be deleted entirely.
No, the functions lprocfs_{rd,wr}_uint are used once through a macro:
ldlm/ldlm_resource.c calls LPROC_SEQ_FOPS_RW_TYPE(ldlm_rw, uint)
to define ldlm_rw_uint_seq_{show,write}, which then calls
lprocfs_{rd,wr}_uint which in turn call seq_printf/kstrtouint_from_user.
It seems like much indirection for almost no gain besides hiding
access to ((seq_file *) file->private_data)->private in a macro.
If you agree, I will prepare a patch that switches ldlm_resource to
using the LPROC_SEQ_FOPS macro directly, which allows us to remove two
trivial wrappers.
/Mathias
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] staging: lustre: lprocfs: Use kstrtouint_from_user
2017-05-18 15:13 ` [lustre-devel] " Mathias Rav
@ 2017-05-19 3:02 ` Dilger, Andreas
-1 siblings, 0 replies; 14+ messages in thread
From: Dilger, Andreas @ 2017-05-19 3:02 UTC (permalink / raw)
To: Mathias Rav
Cc: Greg Kroah-Hartman, devel, Drokin, Oleg, James Simmons,
Linux Kernel Mailing List, Lustre Development List
On May 18, 2017, at 17:13, Mathias Rav <mathiasrav@gmail.com> wrote:
>
> On Thu, 18 May 2017 14:48:25 +0000
> "Dilger, Andreas" <andreas.dilger@intel.com> wrote:
>
>> On May 18, 2017, at 15:53, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
>>>
>>> On Thu, May 04, 2017 at 12:13:38PM -0400, Mathias Rav wrote:
>>>> Prefer kstrtouint_from_user to copy_from_user+simple_strtoul.
>>>>
>>>> The helper function lprocfs_wr_uint() is only used to implement
>>>> "dump_granted_max" in debugfs.
>>>>
>>>> Note the slight change in semantics: The previous implementation using
>>>> simple_strtoul allows garbage after the number, whereas kstrtox only allows
>>>> a trailing line break. The previous implementation allowed a write of zero
>>>> bytes whereas kstrtox will return -EINVAL. Since this only affects a single
>>>> debugfs endpoint, this should be a permissible slight change of semantics
>>>> in exchange for 18 fewer lines of code.
>>>>
>>>> Signed-off-by: Mathias Rav <mathiasrav@gmail.com>
>>>> ---
>>>> .../lustre/lustre/obdclass/lprocfs_status.c | 22 +---------------------
>>>> 1 file changed, 1 insertion(+), 21 deletions(-)
>>>>
>>>> diff --git a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
>>>> index 1ec6e3767d81..338ce34d6514 100644
>>>> --- a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
>>>> +++ b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
>>>> @@ -399,27 +399,7 @@ EXPORT_SYMBOL(lprocfs_rd_uint);
>>>> int lprocfs_wr_uint(struct file *file, const char __user *buffer,
>>>> unsigned long count, void *data)
>>>> {
>>>> - unsigned *p = data;
>>>> - char dummy[MAX_STRING_SIZE + 1], *end;
>>>> - unsigned long tmp;
>>>> -
>>>> - if (count >= sizeof(dummy))
>>>> - return -EINVAL;
>>>> -
>>>> - if (count == 0)
>>>> - return 0;
>>>> -
>>>> - if (copy_from_user(dummy, buffer, count))
>>>> - return -EFAULT;
>>>> -
>>>> - dummy[count] = '\0';
>>>> -
>>>> - tmp = simple_strtoul(dummy, &end, 0);
>>>> - if (dummy == end)
>>>> - return -EINVAL;
>>>> -
>>>> - *p = (unsigned int)tmp;
>>>> - return count;
>>>> + return kstrtouint_from_user(buffer, count, 0, (unsigned int *)data);
>>>
>>> Why not just delete this whole function and have the callers make this
>>> call instead? No need for unneeded wrapper functions of core kernel
>>> calls.
>>
>> Even better, it looks like this function has no callers on the client and could just
>> be deleted entirely.
>
> No, the functions lprocfs_{rd,wr}_uint are used once through a macro:
> ldlm/ldlm_resource.c calls LPROC_SEQ_FOPS_RW_TYPE(ldlm_rw, uint)
> to define ldlm_rw_uint_seq_{show,write}, which then calls
> lprocfs_{rd,wr}_uint which in turn call seq_printf/kstrtouint_from_user.
>
> It seems like much indirection for almost no gain besides hiding
> access to ((seq_file *) file->private_data)->private in a macro.
>
> If you agree, I will prepare a patch that switches ldlm_resource to
> using the LPROC_SEQ_FOPS macro directly, which allows us to remove two
> trivial wrappers.
Please do.
Cheers, Andreas
--
Andreas Dilger
Lustre Principal Architect
Intel Corporation
^ permalink raw reply [flat|nested] 14+ messages in thread
* [lustre-devel] [PATCH 1/2] staging: lustre: lprocfs: Use kstrtouint_from_user
@ 2017-05-19 3:02 ` Dilger, Andreas
0 siblings, 0 replies; 14+ messages in thread
From: Dilger, Andreas @ 2017-05-19 3:02 UTC (permalink / raw)
To: Mathias Rav
Cc: Greg Kroah-Hartman, devel, Drokin, Oleg, James Simmons,
Linux Kernel Mailing List, Lustre Development List
On May 18, 2017, at 17:13, Mathias Rav <mathiasrav@gmail.com> wrote:
>
> On Thu, 18 May 2017 14:48:25 +0000
> "Dilger, Andreas" <andreas.dilger@intel.com> wrote:
>
>> On May 18, 2017, at 15:53, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
>>>
>>> On Thu, May 04, 2017 at 12:13:38PM -0400, Mathias Rav wrote:
>>>> Prefer kstrtouint_from_user to copy_from_user+simple_strtoul.
>>>>
>>>> The helper function lprocfs_wr_uint() is only used to implement
>>>> "dump_granted_max" in debugfs.
>>>>
>>>> Note the slight change in semantics: The previous implementation using
>>>> simple_strtoul allows garbage after the number, whereas kstrtox only allows
>>>> a trailing line break. The previous implementation allowed a write of zero
>>>> bytes whereas kstrtox will return -EINVAL. Since this only affects a single
>>>> debugfs endpoint, this should be a permissible slight change of semantics
>>>> in exchange for 18 fewer lines of code.
>>>>
>>>> Signed-off-by: Mathias Rav <mathiasrav@gmail.com>
>>>> ---
>>>> .../lustre/lustre/obdclass/lprocfs_status.c | 22 +---------------------
>>>> 1 file changed, 1 insertion(+), 21 deletions(-)
>>>>
>>>> diff --git a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
>>>> index 1ec6e3767d81..338ce34d6514 100644
>>>> --- a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
>>>> +++ b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
>>>> @@ -399,27 +399,7 @@ EXPORT_SYMBOL(lprocfs_rd_uint);
>>>> int lprocfs_wr_uint(struct file *file, const char __user *buffer,
>>>> unsigned long count, void *data)
>>>> {
>>>> - unsigned *p = data;
>>>> - char dummy[MAX_STRING_SIZE + 1], *end;
>>>> - unsigned long tmp;
>>>> -
>>>> - if (count >= sizeof(dummy))
>>>> - return -EINVAL;
>>>> -
>>>> - if (count == 0)
>>>> - return 0;
>>>> -
>>>> - if (copy_from_user(dummy, buffer, count))
>>>> - return -EFAULT;
>>>> -
>>>> - dummy[count] = '\0';
>>>> -
>>>> - tmp = simple_strtoul(dummy, &end, 0);
>>>> - if (dummy == end)
>>>> - return -EINVAL;
>>>> -
>>>> - *p = (unsigned int)tmp;
>>>> - return count;
>>>> + return kstrtouint_from_user(buffer, count, 0, (unsigned int *)data);
>>>
>>> Why not just delete this whole function and have the callers make this
>>> call instead? No need for unneeded wrapper functions of core kernel
>>> calls.
>>
>> Even better, it looks like this function has no callers on the client and could just
>> be deleted entirely.
>
> No, the functions lprocfs_{rd,wr}_uint are used once through a macro:
> ldlm/ldlm_resource.c calls LPROC_SEQ_FOPS_RW_TYPE(ldlm_rw, uint)
> to define ldlm_rw_uint_seq_{show,write}, which then calls
> lprocfs_{rd,wr}_uint which in turn call seq_printf/kstrtouint_from_user.
>
> It seems like much indirection for almost no gain besides hiding
> access to ((seq_file *) file->private_data)->private in a macro.
>
> If you agree, I will prepare a patch that switches ldlm_resource to
> using the LPROC_SEQ_FOPS macro directly, which allows us to remove two
> trivial wrappers.
Please do.
Cheers, Andreas
--
Andreas Dilger
Lustre Principal Architect
Intel Corporation
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2017-05-19 3:03 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-04 16:13 [PATCH 0/2] staging: lustre: lprocfs: Fix coding style issues Mathias Rav
2017-05-04 16:13 ` [lustre-devel] " Mathias Rav
2017-05-04 16:13 ` [PATCH 1/2] staging: lustre: lprocfs: Use kstrtouint_from_user Mathias Rav
2017-05-04 16:13 ` [lustre-devel] " Mathias Rav
2017-05-04 16:13 ` [PATCH 2/2] staging: lustre: lprocfs: Use seq_puts Mathias Rav
2017-05-04 16:13 ` [lustre-devel] " Mathias Rav
2017-05-18 13:53 ` [PATCH 1/2] staging: lustre: lprocfs: Use kstrtouint_from_user Greg Kroah-Hartman
2017-05-18 13:53 ` [lustre-devel] " Greg Kroah-Hartman
2017-05-18 14:48 ` Dilger, Andreas
2017-05-18 14:48 ` [lustre-devel] " Dilger, Andreas
2017-05-18 15:13 ` Mathias Rav
2017-05-18 15:13 ` [lustre-devel] " Mathias Rav
2017-05-19 3:02 ` Dilger, Andreas
2017-05-19 3:02 ` [lustre-devel] " Dilger, Andreas
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.