Inorder to notify the user that value is not successfuly set in sys
entry, error should be returned from store function instead of count
Signed-off-by: Saurabh Sengar <[email protected]>
---
drivers/staging/speakup/kobjects.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/speakup/kobjects.c b/drivers/staging/speakup/kobjects.c
index fdfeb42..b3a83fb 100644
--- a/drivers/staging/speakup/kobjects.c
+++ b/drivers/staging/speakup/kobjects.c
@@ -640,7 +640,8 @@ ssize_t spk_var_store(struct kobject *kobj, struct kobj_attribute *attr,
len = E_INC;
else
len = E_SET;
- if (kstrtol(cp, 10, &value) == 0)
+ ret = kstrtol(cp, 10, &value);
+ if (!ret)
ret = spk_set_num_var(value, param, len);
else
pr_warn("overflow or parsing error has occurred");
@@ -688,6 +689,8 @@ ssize_t spk_var_store(struct kobject *kobj, struct kobj_attribute *attr,
if (ret == -ERESTART)
pr_info("%s reset to default value\n", param->name);
+ else if (ret < 0)
+ return ret;
return count;
}
EXPORT_SYMBOL_GPL(spk_var_store);
--
1.9.1
On Fri, Dec 04, 2015 at 08:12:33PM +0530, Saurabh Sengar wrote:
> Inorder to notify the user that value is not successfuly set in sys
> entry, error should be returned from store function instead of count
>
> Signed-off-by: Saurabh Sengar <[email protected]>
> ---
> drivers/staging/speakup/kobjects.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/speakup/kobjects.c b/drivers/staging/speakup/kobjects.c
> index fdfeb42..b3a83fb 100644
> --- a/drivers/staging/speakup/kobjects.c
> +++ b/drivers/staging/speakup/kobjects.c
> @@ -640,7 +640,8 @@ ssize_t spk_var_store(struct kobject *kobj, struct kobj_attribute *attr,
> len = E_INC;
> else
> len = E_SET;
> - if (kstrtol(cp, 10, &value) == 0)
> + ret = kstrtol(cp, 10, &value);
> + if (!ret)
> ret = spk_set_num_var(value, param, len);
Both kstrtol() and spk_set_num_var() return -ERANGE. The next lines
expect that if we got -ERANGE, then it came from spk_set_num_var() so
they print a wrong message.
> else
> pr_warn("overflow or parsing error has occurred");
> @@ -688,6 +689,8 @@ ssize_t spk_var_store(struct kobject *kobj, struct kobj_attribute *attr,
>
> if (ret == -ERESTART)
> pr_info("%s reset to default value\n", param->name);
Is this really true?
This function is so weird and broken. Please look at it some more and
fix it harder with a mallet.
regards,
dan carpenter
On 7 December 2015 at 12:18, Dan Carpenter <[email protected]> wrote:
> On Fri, Dec 04, 2015 at 08:12:33PM +0530, Saurabh Sengar wrote:
>> Inorder to notify the user that value is not successfuly set in sys
>> entry, error should be returned from store function instead of count
>>
>> Signed-off-by: Saurabh Sengar <[email protected]>
>> ---
>> drivers/staging/speakup/kobjects.c | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/speakup/kobjects.c b/drivers/staging/speakup/kobjects.c
>> index fdfeb42..b3a83fb 100644
>> --- a/drivers/staging/speakup/kobjects.c
>> +++ b/drivers/staging/speakup/kobjects.c
>> @@ -640,7 +640,8 @@ ssize_t spk_var_store(struct kobject *kobj, struct kobj_attribute *attr,
>> len = E_INC;
>> else
>> len = E_SET;
>> - if (kstrtol(cp, 10, &value) == 0)
>> + ret = kstrtol(cp, 10, &value);
>> + if (!ret)
>> ret = spk_set_num_var(value, param, len);
>
> Both kstrtol() and spk_set_num_var() return -ERANGE. The next lines
> expect that if we got -ERANGE, then it came from spk_set_num_var() so
> they print a wrong message.
Yes I understand this.
And in case we got -ERANGE from spk_set_num_var, it is printing the
error message.
I have tested this too by passing the out of range values to few parameters.
>
>> else
>> pr_warn("overflow or parsing error has occurred");
>> @@ -688,6 +689,8 @@ ssize_t spk_var_store(struct kobject *kobj, struct kobj_attribute *attr,
>>
>> if (ret == -ERESTART)
>> pr_info("%s reset to default value\n", param->name);
>
> Is this really true?
Sorry, I am not sure here what you mean here.
I have not implemented it.
>
> This function is so weird and broken. Please look at it some more and
> fix it harder with a mallet.
You mean I broke it ?
I don't think so, I have tested the functionality before submitting the patch.
If you mean that this function already not in good shape, I understand
and agree with you.
>
> regards,
> dan carpenter
>
On Mon, Dec 07, 2015 at 12:46:37PM +0530, Saurabh Sengar wrote:
> >> @@ -688,6 +689,8 @@ ssize_t spk_var_store(struct kobject *kobj, struct kobj_attribute *attr,
> >>
> >> if (ret == -ERESTART)
> >> pr_info("%s reset to default value\n", param->name);
> >
> > Is this really true?
> Sorry, I am not sure here what you mean here.
> I have not implemented it.
> >
> > This function is so weird and broken. Please look at it some more and
> > fix it harder with a mallet.
> You mean I broke it ?
No, I mean it was broken to begin with. Write a more extensive patch to
fix it.
That printk should be moved up to where we actually do the reset.
Anyway speakup is actually really bad and there is a lot of broken
stuff so you don't have to fix this function if you don't want to. Just
fix the -ERANGE issue I mentioned and resend if you want.
regards,
dan carpenter
Inorder to notify the user that value is not successfuly set in sys
entry, error should be returned from store function instead of count
Signed-off-by: Saurabh Sengar <[email protected]>
---
v2:
Hi Dan,
I will look more into this function in my free time.
For now just sending you this patch fixing ERANGE as commented
drivers/staging/speakup/kobjects.c | 4 +++-
drivers/staging/staging.c | 4 ++--
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/speakup/kobjects.c b/drivers/staging/speakup/kobjects.c
index fdfeb42..509163c 100644
--- a/drivers/staging/speakup/kobjects.c
+++ b/drivers/staging/speakup/kobjects.c
@@ -640,7 +640,7 @@ ssize_t spk_var_store(struct kobject *kobj, struct kobj_attribute *attr,
len = E_INC;
else
len = E_SET;
- if (kstrtol(cp, 10, &value) == 0)
+ if (!kstrtol(cp, 10, &value))
ret = spk_set_num_var(value, param, len);
else
pr_warn("overflow or parsing error has occurred");
@@ -688,6 +688,8 @@ ssize_t spk_var_store(struct kobject *kobj, struct kobj_attribute *attr,
if (ret == -ERESTART)
pr_info("%s reset to default value\n", param->name);
+ else if (ret < 0)
+ return ret;
return count;
}
EXPORT_SYMBOL_GPL(spk_var_store);
diff --git a/drivers/staging/staging.c b/drivers/staging/staging.c
index 233e589..36dd594 100644
--- a/drivers/staging/staging.c
+++ b/drivers/staging/staging.c
@@ -2,12 +2,12 @@
#include <linux/init.h>
#include <linux/module.h>
-static int __init staging_init(void)
+static int staging_init(void)
{
return 0;
}
-static void __exit staging_exit(void)
+static void staging_exit(void)
{
}
--
1.9.1