The sunrpc debug sysctl files only accept decimal number right now.
But all the XXXDBUG_XXX macros are defined as hexadecimal.
It is not easy to set or check an separate flag.
This patch let those files support accepting hexadecimal number,
(decimal number is also supported). Also, display it as hexadecimal.
Signed-off-by: Kinglong Mee <[email protected]>
---
net/sunrpc/sysctl.c | 26 +++++++++++++++-----------
1 file changed, 15 insertions(+), 11 deletions(-)
diff --git a/net/sunrpc/sysctl.c b/net/sunrpc/sysctl.c
index 887f018..8e85e6f 100644
--- a/net/sunrpc/sysctl.c
+++ b/net/sunrpc/sysctl.c
@@ -76,7 +76,7 @@ static int
proc_dodebug(struct ctl_table *table, int write,
void __user *buffer, size_t *lenp, loff_t *ppos)
{
- char tmpbuf[20], c, *s;
+ char tmpbuf[20], c, *s = NULL;
char __user *p;
unsigned int value;
size_t left, len;
@@ -103,23 +103,27 @@ proc_dodebug(struct ctl_table *table, int write,
return -EFAULT;
tmpbuf[left] = '\0';
- for (s = tmpbuf, value = 0; '0' <= *s && *s <= '9'; s++, left--)
- value = 10 * value + (*s - '0');
- if (*s && !isspace(*s))
- return -EINVAL;
- while (left && isspace(*s))
- left--, s++;
+ if (tmpbuf[0] == '0' && tmpbuf[1] == 'x')
+ value = simple_strtol(tmpbuf, &s, 16);
+ else
+ value = simple_strtol(tmpbuf, &s, 0);
+ if (s) {
+ if (!isspace(*s))
+ return -EINVAL;
+ left -= (s - tmpbuf);
+ while (left && isspace(*s))
+ left--, s++;
+ } else
+ left = 0;
*(unsigned int *) table->data = value;
/* Display the RPC tasks on writing to rpc_debug */
if (strcmp(table->procname, "rpc_debug") == 0)
rpc_show_tasks(&init_net);
} else {
- if (!access_ok(VERIFY_WRITE, buffer, left))
- return -EFAULT;
- len = sprintf(tmpbuf, "%d", *(unsigned int *) table->data);
+ len = sprintf(tmpbuf, "0x%04x", *(unsigned int *) table->data);
if (len > left)
len = left;
- if (__copy_to_user(buffer, tmpbuf, len))
+ if (copy_to_user(buffer, tmpbuf, len))
return -EFAULT;
if ((left -= len) > 0) {
if (put_user('\n', (char __user *)buffer + len))
--
2.5.0
On Thu, Sep 10, 2015 at 08:41:34PM +0800, Kinglong Mee wrote:
> The sunrpc debug sysctl files only accept decimal number right now.
> But all the XXXDBUG_XXX macros are defined as hexadecimal.
> It is not easy to set or check an separate flag.
>
> This patch let those files support accepting hexadecimal number,
> (decimal number is also supported). Also, display it as hexadecimal.
That potentially breaks backwards-compatibility if an program that reads
this file isn't prepared to accept a hexadecimal value.
That said, rpcdebug seems OK (it uses strtoul(.,.,0), which I think
should parse that fine?), and it may well be the only program that
actually parses this, so.... I suppose it's OK with me if it's OK with
Trond.
--b.
>
> Signed-off-by: Kinglong Mee <[email protected]>
> ---
> net/sunrpc/sysctl.c | 26 +++++++++++++++-----------
> 1 file changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/net/sunrpc/sysctl.c b/net/sunrpc/sysctl.c
> index 887f018..8e85e6f 100644
> --- a/net/sunrpc/sysctl.c
> +++ b/net/sunrpc/sysctl.c
> @@ -76,7 +76,7 @@ static int
> proc_dodebug(struct ctl_table *table, int write,
> void __user *buffer, size_t *lenp, loff_t *ppos)
> {
> - char tmpbuf[20], c, *s;
> + char tmpbuf[20], c, *s = NULL;
> char __user *p;
> unsigned int value;
> size_t left, len;
> @@ -103,23 +103,27 @@ proc_dodebug(struct ctl_table *table, int write,
> return -EFAULT;
> tmpbuf[left] = '\0';
>
> - for (s = tmpbuf, value = 0; '0' <= *s && *s <= '9'; s++, left--)
> - value = 10 * value + (*s - '0');
> - if (*s && !isspace(*s))
> - return -EINVAL;
> - while (left && isspace(*s))
> - left--, s++;
> + if (tmpbuf[0] == '0' && tmpbuf[1] == 'x')
> + value = simple_strtol(tmpbuf, &s, 16);
> + else
> + value = simple_strtol(tmpbuf, &s, 0);
> + if (s) {
> + if (!isspace(*s))
> + return -EINVAL;
> + left -= (s - tmpbuf);
> + while (left && isspace(*s))
> + left--, s++;
> + } else
> + left = 0;
> *(unsigned int *) table->data = value;
> /* Display the RPC tasks on writing to rpc_debug */
> if (strcmp(table->procname, "rpc_debug") == 0)
> rpc_show_tasks(&init_net);
> } else {
> - if (!access_ok(VERIFY_WRITE, buffer, left))
> - return -EFAULT;
> - len = sprintf(tmpbuf, "%d", *(unsigned int *) table->data);
> + len = sprintf(tmpbuf, "0x%04x", *(unsigned int *) table->data);
> if (len > left)
> len = left;
> - if (__copy_to_user(buffer, tmpbuf, len))
> + if (copy_to_user(buffer, tmpbuf, len))
> return -EFAULT;
> if ((left -= len) > 0) {
> if (put_user('\n', (char __user *)buffer + len))
> --
> 2.5.0
On 9/12/2015 05:23, J. Bruce Fields wrote:
> On Thu, Sep 10, 2015 at 08:41:34PM +0800, Kinglong Mee wrote:
>> The sunrpc debug sysctl files only accept decimal number right now.
>> But all the XXXDBUG_XXX macros are defined as hexadecimal.
>> It is not easy to set or check an separate flag.
>>
>> This patch let those files support accepting hexadecimal number,
>> (decimal number is also supported). Also, display it as hexadecimal.
>
> That potentially breaks backwards-compatibility if an program that reads
> this file isn't prepared to accept a hexadecimal value.
>
> That said, rpcdebug seems OK (it uses strtoul(.,.,0), which I think
> should parse that fine?), and it may well be the only program that
> actually parses this, so.... I suppose it's OK with me if it's OK with
> Trond.
Thanks for your comments.
I have test of rpcdebug, it's OK when showing the debug flags.
>
> --b.
>
>>
>> Signed-off-by: Kinglong Mee <[email protected]>
>> ---
>> net/sunrpc/sysctl.c | 26 +++++++++++++++-----------
>> 1 file changed, 15 insertions(+), 11 deletions(-)
>>
>> diff --git a/net/sunrpc/sysctl.c b/net/sunrpc/sysctl.c
>> index 887f018..8e85e6f 100644
>> --- a/net/sunrpc/sysctl.c
>> +++ b/net/sunrpc/sysctl.c
>> @@ -76,7 +76,7 @@ static int
>> proc_dodebug(struct ctl_table *table, int write,
>> void __user *buffer, size_t *lenp, loff_t *ppos)
>> {
>> - char tmpbuf[20], c, *s;
>> + char tmpbuf[20], c, *s = NULL;
>> char __user *p;
>> unsigned int value;
>> size_t left, len;
>> @@ -103,23 +103,27 @@ proc_dodebug(struct ctl_table *table, int write,
>> return -EFAULT;
>> tmpbuf[left] = '\0';
>>
>> - for (s = tmpbuf, value = 0; '0' <= *s && *s <= '9'; s++, left--)
>> - value = 10 * value + (*s - '0');
>> - if (*s && !isspace(*s))
>> - return -EINVAL;
>> - while (left && isspace(*s))
>> - left--, s++;
>> + if (tmpbuf[0] == '0' && tmpbuf[1] == 'x')
>> + value = simple_strtol(tmpbuf, &s, 16);
It's a duplicate of this.
>> + else
>> + value = simple_strtol(tmpbuf, &s, 0);
>> + if (s) {
>> + if (!isspace(*s))
It's a bug of checking '\0' here.
I will fix those problems in new version.
thanks,
Kinglong Mee
>> + return -EINVAL;
>> + left -= (s - tmpbuf);
>> + while (left && isspace(*s))
>> + left--, s++;
>> + } else
>> + left = 0;
>> *(unsigned int *) table->data = value;
>> /* Display the RPC tasks on writing to rpc_debug */
>> if (strcmp(table->procname, "rpc_debug") == 0)
>> rpc_show_tasks(&init_net);
>> } else {
>> - if (!access_ok(VERIFY_WRITE, buffer, left))
>> - return -EFAULT;
>> - len = sprintf(tmpbuf, "%d", *(unsigned int *) table->data);
>> + len = sprintf(tmpbuf, "0x%04x", *(unsigned int *) table->data);
>> if (len > left)
>> len = left;
>> - if (__copy_to_user(buffer, tmpbuf, len))
>> + if (copy_to_user(buffer, tmpbuf, len))
>> return -EFAULT;
>> if ((left -= len) > 0) {
>> if (put_user('\n', (char __user *)buffer + len))
>> --
>> 2.5.0
>
The sunrpc debug sysctl files only accept decimal number right now.
But all the XXXDBUG_XXX macros are defined as hexadecimal.
It is not easy to set or check an separate flag.
This patch let those files support accepting hexadecimal number,
(decimal number is also supported). Also, display it as hexadecimal.
v2,
Remove duplicate parsing of '0x...', just using simple_strtol(tmpbuf, &s, 0)
Fix a bug of isspace() checking after parsing
Signed-off-by: Kinglong Mee <[email protected]>
---
net/sunrpc/sysctl.c | 23 ++++++++++++-----------
1 file changed, 12 insertions(+), 11 deletions(-)
diff --git a/net/sunrpc/sysctl.c b/net/sunrpc/sysctl.c
index 887f018..c88d9bc 100644
--- a/net/sunrpc/sysctl.c
+++ b/net/sunrpc/sysctl.c
@@ -76,7 +76,7 @@ static int
proc_dodebug(struct ctl_table *table, int write,
void __user *buffer, size_t *lenp, loff_t *ppos)
{
- char tmpbuf[20], c, *s;
+ char tmpbuf[20], c, *s = NULL;
char __user *p;
unsigned int value;
size_t left, len;
@@ -103,23 +103,24 @@ proc_dodebug(struct ctl_table *table, int write,
return -EFAULT;
tmpbuf[left] = '\0';
- for (s = tmpbuf, value = 0; '0' <= *s && *s <= '9'; s++, left--)
- value = 10 * value + (*s - '0');
- if (*s && !isspace(*s))
- return -EINVAL;
- while (left && isspace(*s))
- left--, s++;
+ value = simple_strtol(tmpbuf, &s, 0);
+ if (s) {
+ left -= (s - tmpbuf);
+ if (left && !isspace(*s))
+ return -EINVAL;
+ while (left && isspace(*s))
+ left--, s++;
+ } else
+ left = 0;
*(unsigned int *) table->data = value;
/* Display the RPC tasks on writing to rpc_debug */
if (strcmp(table->procname, "rpc_debug") == 0)
rpc_show_tasks(&init_net);
} else {
- if (!access_ok(VERIFY_WRITE, buffer, left))
- return -EFAULT;
- len = sprintf(tmpbuf, "%d", *(unsigned int *) table->data);
+ len = sprintf(tmpbuf, "0x%04x", *(unsigned int *) table->data);
if (len > left)
len = left;
- if (__copy_to_user(buffer, tmpbuf, len))
+ if (copy_to_user(buffer, tmpbuf, len))
return -EFAULT;
if ((left -= len) > 0) {
if (put_user('\n', (char __user *)buffer + len))
--
2.5.0
On Fri, Sep 11, 2015 at 9:37 PM, Kinglong Mee <[email protected]> wrote:
> The sunrpc debug sysctl files only accept decimal number right now.
> But all the XXXDBUG_XXX macros are defined as hexadecimal.
> It is not easy to set or check an separate flag.
>
> This patch let those files support accepting hexadecimal number,
> (decimal number is also supported). Also, display it as hexadecimal.
>
> v2,
> Remove duplicate parsing of '0x...', just using simple_strtol(tmpbuf, &s, 0)
> Fix a bug of isspace() checking after parsing
>
> Signed-off-by: Kinglong Mee <[email protected]>
> ---
> net/sunrpc/sysctl.c | 23 ++++++++++++-----------
> 1 file changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/net/sunrpc/sysctl.c b/net/sunrpc/sysctl.c
> index 887f018..c88d9bc 100644
> --- a/net/sunrpc/sysctl.c
> +++ b/net/sunrpc/sysctl.c
> @@ -76,7 +76,7 @@ static int
> proc_dodebug(struct ctl_table *table, int write,
> void __user *buffer, size_t *lenp, loff_t *ppos)
> {
> - char tmpbuf[20], c, *s;
> + char tmpbuf[20], c, *s = NULL;
> char __user *p;
> unsigned int value;
> size_t left, len;
> @@ -103,23 +103,24 @@ proc_dodebug(struct ctl_table *table, int write,
> return -EFAULT;
> tmpbuf[left] = '\0';
>
> - for (s = tmpbuf, value = 0; '0' <= *s && *s <= '9'; s++, left--)
> - value = 10 * value + (*s - '0');
> - if (*s && !isspace(*s))
> - return -EINVAL;
> - while (left && isspace(*s))
> - left--, s++;
> + value = simple_strtol(tmpbuf, &s, 0);
> + if (s) {
> + left -= (s - tmpbuf);
> + if (left && !isspace(*s))
> + return -EINVAL;
> + while (left && isspace(*s))
> + left--, s++;
> + } else
> + left = 0;
> *(unsigned int *) table->data = value;
> /* Display the RPC tasks on writing to rpc_debug */
> if (strcmp(table->procname, "rpc_debug") == 0)
> rpc_show_tasks(&init_net);
> } else {
> - if (!access_ok(VERIFY_WRITE, buffer, left))
> - return -EFAULT;
> - len = sprintf(tmpbuf, "%d", *(unsigned int *) table->data);
> + len = sprintf(tmpbuf, "0x%04x", *(unsigned int *) table->data);
> if (len > left)
> len = left;
> - if (__copy_to_user(buffer, tmpbuf, len))
> + if (copy_to_user(buffer, tmpbuf, len))
> return -EFAULT;
> if ((left -= len) > 0) {
> if (put_user('\n', (char __user *)buffer + len))
> --
> 2.5.0
>
Bruce, did you take this patch, or is there an expectation that I should do so?
Cheers
Trond
On Tue, Nov 03, 2015 at 12:38:19PM -0500, Trond Myklebust wrote:
> On Fri, Sep 11, 2015 at 9:37 PM, Kinglong Mee <[email protected]> wrote:
> > The sunrpc debug sysctl files only accept decimal number right now.
> > But all the XXXDBUG_XXX macros are defined as hexadecimal.
> > It is not easy to set or check an separate flag.
> >
> > This patch let those files support accepting hexadecimal number,
> > (decimal number is also supported). Also, display it as hexadecimal.
> >
> > v2,
> > Remove duplicate parsing of '0x...', just using simple_strtol(tmpbuf, &s, 0)
> > Fix a bug of isspace() checking after parsing
> >
> > Signed-off-by: Kinglong Mee <[email protected]>
> > ---
> > net/sunrpc/sysctl.c | 23 ++++++++++++-----------
> > 1 file changed, 12 insertions(+), 11 deletions(-)
> >
> > diff --git a/net/sunrpc/sysctl.c b/net/sunrpc/sysctl.c
> > index 887f018..c88d9bc 100644
> > --- a/net/sunrpc/sysctl.c
> > +++ b/net/sunrpc/sysctl.c
> > @@ -76,7 +76,7 @@ static int
> > proc_dodebug(struct ctl_table *table, int write,
> > void __user *buffer, size_t *lenp, loff_t *ppos)
> > {
> > - char tmpbuf[20], c, *s;
> > + char tmpbuf[20], c, *s = NULL;
> > char __user *p;
> > unsigned int value;
> > size_t left, len;
> > @@ -103,23 +103,24 @@ proc_dodebug(struct ctl_table *table, int write,
> > return -EFAULT;
> > tmpbuf[left] = '\0';
> >
> > - for (s = tmpbuf, value = 0; '0' <= *s && *s <= '9'; s++, left--)
> > - value = 10 * value + (*s - '0');
> > - if (*s && !isspace(*s))
> > - return -EINVAL;
> > - while (left && isspace(*s))
> > - left--, s++;
> > + value = simple_strtol(tmpbuf, &s, 0);
> > + if (s) {
> > + left -= (s - tmpbuf);
> > + if (left && !isspace(*s))
> > + return -EINVAL;
> > + while (left && isspace(*s))
> > + left--, s++;
> > + } else
> > + left = 0;
> > *(unsigned int *) table->data = value;
> > /* Display the RPC tasks on writing to rpc_debug */
> > if (strcmp(table->procname, "rpc_debug") == 0)
> > rpc_show_tasks(&init_net);
> > } else {
> > - if (!access_ok(VERIFY_WRITE, buffer, left))
> > - return -EFAULT;
> > - len = sprintf(tmpbuf, "%d", *(unsigned int *) table->data);
> > + len = sprintf(tmpbuf, "0x%04x", *(unsigned int *) table->data);
> > if (len > left)
> > len = left;
> > - if (__copy_to_user(buffer, tmpbuf, len))
> > + if (copy_to_user(buffer, tmpbuf, len))
> > return -EFAULT;
> > if ((left -= len) > 0) {
> > if (put_user('\n', (char __user *)buffer + len))
> > --
> > 2.5.0
> >
>
> Bruce, did you take this patch, or is there an expectation that I should do so?
Patch looked OK to me, but I didn't take it, could you?
--b.
On Tue, Nov 3, 2015 at 2:32 PM, J. Bruce Fields <[email protected]> wrote:
> On Tue, Nov 03, 2015 at 12:38:19PM -0500, Trond Myklebust wrote:
>> On Fri, Sep 11, 2015 at 9:37 PM, Kinglong Mee <[email protected]> wrote:
>> > The sunrpc debug sysctl files only accept decimal number right now.
>> > But all the XXXDBUG_XXX macros are defined as hexadecimal.
>> > It is not easy to set or check an separate flag.
>> >
>> > This patch let those files support accepting hexadecimal number,
>> > (decimal number is also supported). Also, display it as hexadecimal.
>> >
>> > v2,
>> > Remove duplicate parsing of '0x...', just using simple_strtol(tmpbuf, &s, 0)
>> > Fix a bug of isspace() checking after parsing
>> >
>> > Signed-off-by: Kinglong Mee <[email protected]>
>> > ---
>> > net/sunrpc/sysctl.c | 23 ++++++++++++-----------
>> > 1 file changed, 12 insertions(+), 11 deletions(-)
>> >
>> > diff --git a/net/sunrpc/sysctl.c b/net/sunrpc/sysctl.c
>> > index 887f018..c88d9bc 100644
>> > --- a/net/sunrpc/sysctl.c
>> > +++ b/net/sunrpc/sysctl.c
>> > @@ -76,7 +76,7 @@ static int
>> > proc_dodebug(struct ctl_table *table, int write,
>> > void __user *buffer, size_t *lenp, loff_t *ppos)
>> > {
>> > - char tmpbuf[20], c, *s;
>> > + char tmpbuf[20], c, *s = NULL;
>> > char __user *p;
>> > unsigned int value;
>> > size_t left, len;
>> > @@ -103,23 +103,24 @@ proc_dodebug(struct ctl_table *table, int write,
>> > return -EFAULT;
>> > tmpbuf[left] = '\0';
>> >
>> > - for (s = tmpbuf, value = 0; '0' <= *s && *s <= '9'; s++, left--)
>> > - value = 10 * value + (*s - '0');
>> > - if (*s && !isspace(*s))
>> > - return -EINVAL;
>> > - while (left && isspace(*s))
>> > - left--, s++;
>> > + value = simple_strtol(tmpbuf, &s, 0);
>> > + if (s) {
>> > + left -= (s - tmpbuf);
>> > + if (left && !isspace(*s))
>> > + return -EINVAL;
>> > + while (left && isspace(*s))
>> > + left--, s++;
>> > + } else
>> > + left = 0;
>> > *(unsigned int *) table->data = value;
>> > /* Display the RPC tasks on writing to rpc_debug */
>> > if (strcmp(table->procname, "rpc_debug") == 0)
>> > rpc_show_tasks(&init_net);
>> > } else {
>> > - if (!access_ok(VERIFY_WRITE, buffer, left))
>> > - return -EFAULT;
>> > - len = sprintf(tmpbuf, "%d", *(unsigned int *) table->data);
>> > + len = sprintf(tmpbuf, "0x%04x", *(unsigned int *) table->data);
>> > if (len > left)
>> > len = left;
>> > - if (__copy_to_user(buffer, tmpbuf, len))
>> > + if (copy_to_user(buffer, tmpbuf, len))
>> > return -EFAULT;
>> > if ((left -= len) > 0) {
>> > if (put_user('\n', (char __user *)buffer + len))
>> > --
>> > 2.5.0
>> >
>>
>> Bruce, did you take this patch, or is there an expectation that I should do so?
>
> Patch looked OK to me, but I didn't take it, could you?
>
Sure. I'll pick it up.