Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1283876 > unrolled thread

[PATCH] Staging: speakup: kobjects: Return the error type to caller

Back to article view | Back to linux.kernel

Contents

  [PATCH] Staging: speakup: kobjects: Return the error type to caller Saurabh Sengar <saurabh.truth@gmail.com> - 2015-12-04 15:50 +0100
    Re: [PATCH] Staging: speakup: kobjects: Return the error type to  caller Dan Carpenter <dan.carpenter@oracle.com> - 2015-12-07 07:50 +0100
      Re: [PATCH] Staging: speakup: kobjects: Return the error type to caller Saurabh Sengar <saurabh.truth@gmail.com> - 2015-12-07 08:20 +0100
        Re: [PATCH] Staging: speakup: kobjects: Return the error type to  caller Dan Carpenter <dan.carpenter@oracle.com> - 2015-12-07 09:40 +0100
          [PATCH v2] Staging: speakup: kobjects: Return the error type to caller Saurabh Sengar <saurabh.truth@gmail.com> - 2015-12-07 11:10 +0100

#1283876 — [PATCH] Staging: speakup: kobjects: Return the error type to caller

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 <saurabh.truth@gmail.com>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1285031 — Re: [PATCH] Staging: speakup: kobjects: Return the error type to caller

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 <saurabh.truth@gmail.com>
> ---
>  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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1285049

On 7 December 2015 at 12:18, Dan Carpenter <dan.carpenter@oracle.com> 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 <saurabh.truth@gmail.com>
>> ---
>>  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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1285084 — Re: [PATCH] Staging: speakup: kobjects: Return the error type to caller

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1285165 — [PATCH v2] Staging: speakup: kobjects: Return the error type to caller

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 <saurabh.truth@gmail.com>
---
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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web