[PATCH 1/1] staging/speakup/kobjects.c: Code improvement.

[PATCH 1/1] staging/speakup/kobjects.c: Code improvement.

[Raphael S.Carvalho]

Well, there is no need to use strcmp since we can make a test of similar semantic by using the var_id field of param.
I moved the test into the VAR_NUM:VAR_TIME case since VAR_STRING will never be "voice".

spk_xlate isn't used anymore (in line 628), then there is no difference between using cp and buf in VAR_STRING case.
Besides, buf is a const char and those changes remove one uneeded line.

I created the function spk_reset_default_value because it clarifies the code and allows code reusing.

Signed-off-by: Raphael S.Carvalho <raphael.scarv@gmail.com>
---
 drivers/staging/speakup/kobjects.c |   71 ++++++++++++++++++++----------------
 1 files changed, 39 insertions(+), 32 deletions(-)

diff --git a/drivers/staging/speakup/kobjects.c b/drivers/staging/speakup/kobjects.c
index 51bdea3..5c6e77a 100644
--- a/drivers/staging/speakup/kobjects.c
+++ b/drivers/staging/speakup/kobjects.c
@@ -586,6 +586,25 @@ ssize_t spk_var_show(struct kobject *kobj, struct kobj_attribute *attr,
 EXPORT_SYMBOL_GPL(spk_var_show);
 
 /*
+ * Used to reset either default_pitch or default_vol.
+ */
+static inline void spk_reset_default_value(char *header_name,
+					int *synth_default_value, int idx)
+{
+	struct st_var_header *param;
+
+	if (synth && synth_default_value) {
+		param = spk_var_header_by_name(header_name);
+		if (param)  {
+			spk_set_num_var(synth_default_value[idx],
+					param, E_NEW_DEFAULT);
+			spk_set_num_var(0, param, E_DEFAULT);
+			pr_info("%s reset to default value\n", param->name);
+		}
+	}
+}
+
+/*
  * This function is called when a user echos a value to one of the
  * variable parameters.
  */
@@ -624,56 +643,44 @@ ssize_t spk_var_store(struct kobject *kobj, struct kobj_attribute *attr,
 		if (ret == -ERANGE) {
 			var_data = param->data;
 			pr_warn("value for %s out of range, expect %d to %d\n",
-				attr->attr.name,
+				param->name,
 				var_data->u.n.low, var_data->u.n.high);
 		}
+
+	       /*
+		* If voice was just changed, we might need to reset our default
+		* pitch and volume.
+		*/
+		if (param->var_id == VOICE) {
+			spk_reset_default_value("pitch", synth->default_pitch,
+				value);
+			spk_reset_default_value("vol", synth->default_vol,
+				value);
+		}
 		break;
 	case VAR_STRING:
-		len = strlen(buf);
-		if ((len >= 1) && (buf[len - 1] == '\n'))
+		len = strlen(cp);
+		if ((len >= 1) && (cp[len - 1] == '\n'))
 			--len;
-		if ((len >= 2) && (buf[0] == '"') && (buf[len - 1] == '"')) {
-			++buf;
+		if ((len >= 2) && (cp[0] == '"') && (cp[len - 1] == '"')) {
+			++cp;
 			len -= 2;
 		}
-		cp = (char *) buf;
 		cp[len] = '\0';
-		ret = spk_set_string_var(buf, param, len);
+		ret = spk_set_string_var(cp, param, len);
 		if (ret == -E2BIG)
 			pr_warn("value too long for %s\n",
-					attr->attr.name);
+					param->name);
 		break;
 	default:
 		pr_warn("%s unknown type %d\n",
 			param->name, (int)param->var_type);
 	break;
-	}
-	/*
-	 * If voice was just changed, we might need to reset our default
-	 * pitch and volume.
-	 */
-	if (strcmp(attr->attr.name, "voice") == 0) {
-		if (synth && synth->default_pitch) {
-			param = spk_var_header_by_name("pitch");
-			if (param)  {
-				spk_set_num_var(synth->default_pitch[value],
-						param, E_NEW_DEFAULT);
-				spk_set_num_var(0, param, E_DEFAULT);
-			}
-		}
-		if (synth && synth->default_vol) {
-			param = spk_var_header_by_name("vol");
-			if (param)  {
-				spk_set_num_var(synth->default_vol[value],
-						param, E_NEW_DEFAULT);
-				spk_set_num_var(0, param, E_DEFAULT);
-			}
-		}
-	}
+	}
 	spin_unlock_irqrestore(&speakup_info.spinlock, flags);
 
 	if (ret == -ERESTART)
-		pr_info("%s reset to default value\n", attr->attr.name);
+		pr_info("%s reset to default value\n", param->name);
 	return count;
 }
 EXPORT_SYMBOL_GPL(spk_var_store);
-- 
1.7.2.5

Re: [PATCH 1/1] staging/speakup/kobjects.c: Code improvement.

[Chris Brannon]

"Raphael S.Carvalho" <raphael.scarv@gmail.com> writes:

> +             /*
> +		* If voice was just changed, we might need to reset our default
> +		* pitch and volume.
> +		*/
> +		if (param->var_id == VOICE) {
> +			spk_reset_default_value("pitch", synth->default_pitch,
> +				value);
> +			spk_reset_default_value("vol", synth->default_vol,
> +				value);

There's an "invalid read" bug here.  You didn't introduce it; it has
been there all along.  It's possible that value contains a value that is
out of range, in which case, the spk_reset_default_value calls could
fetch invalid data.  The value of ret should be sufficient for
determining whether value is in range, so I'd change the condition of
the if statement to this:

		if (param->var_id == VOICE && ret != -ERANGE) {

Or possibly better:
		if (param->var_id == VOICE && ret == 0) {

I'd say please resend with that fix, or if not, I can send a one-line
patch to be applied after yours.

-- Chris

Fwd: [PATCH 1/1] staging/speakup/kobjects.c: Code improvement.

[Raphael S Carvalho]

On Mon, Sep 9, 2013 at 12:56 AM, Chris Brannon <chris@the-brannons.com> wrote:
>
> "Raphael S.Carvalho" <raphael.scarv@gmail.com> writes:
>
> > +             /*
> > +             * If voice was just changed, we might need to reset our default
> > +             * pitch and volume.
> > +             */
> > +             if (param->var_id == VOICE) {
> > +                     spk_reset_default_value("pitch", synth->default_pitch,
> > +                             value);
> > +                     spk_reset_default_value("vol", synth->default_vol,
> > +                             value);
>
> There's an "invalid read" bug here.  You didn't introduce it; it has
> been there all along.  It's possible that value contains a value that is
> out of range, in which case, the spk_reset_default_value calls could
> fetch invalid data.  The value of ret should be sufficient for
> determining whether value is in range, so I'd change the condition of
> the if statement to this:


Wouldn't the following code (right before the statement: if
(param->var_id == VOICE))
check if value is out of range?

        value = simple_strtol(cp, NULL, 10);
        ret = spk_set_num_var(value, param, len);

        if (ret == -ERANGE) {
            var_data = param->data;
            pr_warn("value for %s out of range, expect %d to %d\n",
                param->name,
                var_data->u.n.low, var_data->u.n.high);
        }

>
>
>                 if (param->var_id == VOICE && ret != -ERANGE) {
>
> Or possibly better:
>                 if (param->var_id == VOICE && ret == 0) {
>
> I'd say please resend with that fix, or if not, I can send a one-line
> patch to be applied after yours.
>
> -- Chris


Regards,
Raphael S. Carvalho

Re: [PATCH 1/1] staging/speakup/kobjects.c: Code improvement.

[Chris Brannon]

Raphael S Carvalho <raphael.scarv@gmail.com> writes:

> Wouldn't the following code (right before the statement: if
> (param->var_id == VOICE)) 
> check if value is out of range?
>
> value = simple_strtol(cp, NULL, 10);
> ret = spk_set_num_var(value, param, len);
> if (ret == -ERANGE) {
> var_data = param->data;
> pr_warn("value for %s out of range, expect %d to %d\n",
> param->name,
> var_data->u.n.low, var_data->u.n.high);
> }

That code prints an error message if the value is out of range.  Also,
since spk_set_num_var returns -ERANGE, we know that spk_set_num_var
didn't set anything.
But we use value again later in the function, if param->var_id ==
VOICE.  In that second use, we don't check to see if value is in range.
So if I set voice to something nonsensical, like 234567, an error
message will be printed, but the calls in the body of the if statement
will use the nonsense value, reading data from an invalid location.
It seems that there's another bug lurking in this code.
If we try to set voice to default, spk_set_num_var returns -ERESTART.
In this case, we shouldn't use value at all when setting the pitch and volume.
"value" is meaningless, regardless of what it contains.
We should use the value of the default voice as the index instead.  So
the following should be correct, and you can ignore what I suggested earlier.

if (param->var_id == VOICE && (ret == 0 || ret == -ERESTART)) {
	if (ret == -ERESTART)
		value = param->data.u.n.default_val;
	spk_reset_default_value("pitch", synth->default_pitch,
		value);
	spk_reset_default_value("vol", synth->default_vol,
		value);
}

-- Chris

Re: [PATCH 1/1] staging/speakup/kobjects.c: Code improvement.

[Dan Carpenter]

Good eye for spotting the memory corruption bug!

This is a bug fix, so the fix should go in a separate patch and not
merged with a code cleanup patch.  Ordinary users can trigger this so
it's a security bug and separating it out is extra important.

The checking in spk_set_num_var() is not sufficient as well.  If we use
E_INC then we can hit an integer overflow bug:

drivers/staging/speakup/varhandlers.c
   198                  if (how == E_SET)
   199                          val = input;
   200                  else
   201                          val = var_data->u.n.value;
   202                  if (how == E_INC)
   203                          val += input;

"input" comes from the user.  This addition can overflow so that input
is a very high number and now "val" is a low enough number.

   204                  else if (how == E_DEC)
   205                          val -= input;
   206                  if (val < var_data->u.n.low || val > var_data->u.n.high)
   207                          return -ERANGE;

"val" is valid, but "input" is not valid.  We use "input" in the caller
function as the index to an array.

   208          }

I guess that's simple enough to fix but why is the caller using "input"
instead of "val"?

regards,
dan carpenter

Re: [PATCH 1/1] staging/speakup/kobjects.c: Code improvement.

[Chris Brannon]

Dan Carpenter <dan.carpenter@oracle.com> writes:

> Good eye for spotting the memory corruption bug!
>
> This is a bug fix, so the fix should go in a separate patch and not
> merged with a code cleanup patch.  Ordinary users can trigger this so
> it's a security bug and separating it out is extra important.

Ok.  I just sent up a patch to the driverdev list.  I missed a few
of the Cc's that were on this thread, though.
Also, it will conflict with Raphael's cleanup.

> The checking in spk_set_num_var() is not sufficient as well.  If we use
> E_INC then we can hit an integer overflow bug:

Good catch.  In fact, we shouldn't be using input at all!  Instead, we
need to use the value of the voice parameter after it was changed.  That
will be a valid index into the two tables.  My patch does so.

-- Chris

Re: [PATCH 1/1] staging/speakup/kobjects.c: Code improvement.

[Dan Carpenter]

On Tue, Sep 10, 2013 at 06:29:39PM -0700, Chris Brannon wrote:
> Ok.  I just sent up a patch to the driverdev list.  I missed a few
> of the Cc's that were on this thread, though.
> Also, it will conflict with Raphael's cleanup.

You're missing Raphael's CC in particular...

Really, Raphael's patch arrived first.  No one seems to have any
objections to his patch.  Normally apply it first and then apply this
one.  If we decide to push this one to -stable then we would redo it (in
other words push the one you just sent).

Of course, the merge window is open right now so nothing is going to be
applied for a couple weeks.

regards,
dan carpenter

Re: [PATCH 1/1] staging/speakup/kobjects.c: Code improvement.

[Samuel Thibault]

Raphael S.Carvalho, le Mon 02 Sep 2013 19:20:18 -0300, a écrit :
> Well, there is no need to use strcmp since we can make a test of similar semantic by using the var_id field of param.
> I moved the test into the VAR_NUM:VAR_TIME case since VAR_STRING will never be "voice".
> 
> spk_xlate isn't used anymore (in line 628), then there is no difference between using cp and buf in VAR_STRING case.
> Besides, buf is a const char and those changes remove one uneeded line.
> 
> I created the function spk_reset_default_value because it clarifies the code and allows code reusing.
> 
> Signed-off-by: Raphael S.Carvalho <raphael.scarv@gmail.com>

Acked-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

> ---
>  drivers/staging/speakup/kobjects.c |   71 ++++++++++++++++++++----------------
>  1 files changed, 39 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/staging/speakup/kobjects.c b/drivers/staging/speakup/kobjects.c
> index 51bdea3..5c6e77a 100644
> --- a/drivers/staging/speakup/kobjects.c
> +++ b/drivers/staging/speakup/kobjects.c
> @@ -586,6 +586,25 @@ ssize_t spk_var_show(struct kobject *kobj, struct kobj_attribute *attr,
>  EXPORT_SYMBOL_GPL(spk_var_show);
>  
>  /*
> + * Used to reset either default_pitch or default_vol.
> + */
> +static inline void spk_reset_default_value(char *header_name,
> +					int *synth_default_value, int idx)
> +{
> +	struct st_var_header *param;
> +
> +	if (synth && synth_default_value) {
> +		param = spk_var_header_by_name(header_name);
> +		if (param)  {
> +			spk_set_num_var(synth_default_value[idx],
> +					param, E_NEW_DEFAULT);
> +			spk_set_num_var(0, param, E_DEFAULT);
> +			pr_info("%s reset to default value\n", param->name);
> +		}
> +	}
> +}
> +
> +/*
>   * This function is called when a user echos a value to one of the
>   * variable parameters.
>   */
> @@ -624,56 +643,44 @@ ssize_t spk_var_store(struct kobject *kobj, struct kobj_attribute *attr,
>  		if (ret == -ERANGE) {
>  			var_data = param->data;
>  			pr_warn("value for %s out of range, expect %d to %d\n",
> -				attr->attr.name,
> +				param->name,
>  				var_data->u.n.low, var_data->u.n.high);
>  		}
> +
> +	       /*
> +		* If voice was just changed, we might need to reset our default
> +		* pitch and volume.
> +		*/
> +		if (param->var_id == VOICE) {
> +			spk_reset_default_value("pitch", synth->default_pitch,
> +				value);
> +			spk_reset_default_value("vol", synth->default_vol,
> +				value);
> +		}
>  		break;
>  	case VAR_STRING:
> -		len = strlen(buf);
> -		if ((len >= 1) && (buf[len - 1] == '\n'))
> +		len = strlen(cp);
> +		if ((len >= 1) && (cp[len - 1] == '\n'))
>  			--len;
> -		if ((len >= 2) && (buf[0] == '"') && (buf[len - 1] == '"')) {
> -			++buf;
> +		if ((len >= 2) && (cp[0] == '"') && (cp[len - 1] == '"')) {
> +			++cp;
>  			len -= 2;
>  		}
> -		cp = (char *) buf;
>  		cp[len] = '\0';
> -		ret = spk_set_string_var(buf, param, len);
> +		ret = spk_set_string_var(cp, param, len);
>  		if (ret == -E2BIG)
>  			pr_warn("value too long for %s\n",
> -					attr->attr.name);
> +					param->name);
>  		break;
>  	default:
>  		pr_warn("%s unknown type %d\n",
>  			param->name, (int)param->var_type);
>  	break;
> -	}
> -	/*
> -	 * If voice was just changed, we might need to reset our default
> -	 * pitch and volume.
> -	 */
> -	if (strcmp(attr->attr.name, "voice") == 0) {
> -		if (synth && synth->default_pitch) {
> -			param = spk_var_header_by_name("pitch");
> -			if (param)  {
> -				spk_set_num_var(synth->default_pitch[value],
> -						param, E_NEW_DEFAULT);
> -				spk_set_num_var(0, param, E_DEFAULT);
> -			}
> -		}
> -		if (synth && synth->default_vol) {
> -			param = spk_var_header_by_name("vol");
> -			if (param)  {
> -				spk_set_num_var(synth->default_vol[value],
> -						param, E_NEW_DEFAULT);
> -				spk_set_num_var(0, param, E_DEFAULT);
> -			}
> -		}
> -	}
> +	}
>  	spin_unlock_irqrestore(&speakup_info.spinlock, flags);
>  
>  	if (ret == -ERESTART)
> -		pr_info("%s reset to default value\n", attr->attr.name);
> +		pr_info("%s reset to default value\n", param->name);
>  	return count;
>  }
>  EXPORT_SYMBOL_GPL(spk_var_store);
> -- 
> 1.7.2.5
> 

-- 
Samuel
AUTHOR
     FvwmM4 is the result of a random  bit  mutation  on  a  hard
     disk,  presumably  a  result  of  a  cosmic-ray or some such
     thing.
(extrait de la page de man de FvwmM4)

[PATCH 1/1] staging/speakup/kobjects.c: Code improvement.

[Raphael S. Carvalho]

Well, there is no need to use strcmp since we can make a test of similar semantic by using the var_id field of param.
I moved the test into the VAR_NUM:VAR_TIME case since VAR_STRING will never be "voice".

spk_xlate isn't used anymore (in line 608), then there is no difference between using cp and buf in VAR_STRING case.
Besides, buf is a const char and those changes remove one uneeded line.

I created the function spk_reset_default_value because it clarifies the code and allows code reusing.

Signed-off-by: Raphael S. Carvalho <raphael.scarv@gmail.com>
---
 drivers/staging/speakup/kobjects.c |   73 +++++++++++++++++++----------------
 1 files changed, 40 insertions(+), 33 deletions(-)

diff --git a/drivers/staging/speakup/kobjects.c b/drivers/staging/speakup/kobjects.c
index 943b6c1..4660ce3 100644
--- a/drivers/staging/speakup/kobjects.c
+++ b/drivers/staging/speakup/kobjects.c
@@ -585,6 +585,25 @@ ssize_t spk_var_show(struct kobject *kobj, struct kobj_attribute *attr,
 }
 EXPORT_SYMBOL_GPL(spk_var_show);
 
+/*
+ * Used to reset either default_pitch or default_vol.
+ */
+static inline void spk_reset_default_value(char *header_name,
+					int *synth_default_value, int idx)
+{
+	struct st_var_header *param;
+
+	if (synth && synth_default_value) {
+		param = spk_var_header_by_name(header_name);
+		if (param)  {
+			spk_set_num_var(synth_default_value[idx],
+					param, E_NEW_DEFAULT);
+			spk_set_num_var(0, param, E_DEFAULT);
+			pr_info("%s reset to default value\n", param->name);
+		}
+	}
+}
+
 /*
  * This function is called when a user echos a value to one of the
  * variable parameters.
@@ -624,56 +643,44 @@ ssize_t spk_var_store(struct kobject *kobj, struct kobj_attribute *attr,
 		if (ret == -ERANGE) {
 			var_data = param->data;
 			pr_warn("value for %s out of range, expect %d to %d\n",
-				attr->attr.name,
+				param->name,
 				var_data->u.n.low, var_data->u.n.high);
 		}
+
+	       /*
+		* If voice was just changed, we might need to reset our default
+		* pitch and volume.
+		*/
+		if (param->var_id == VOICE) {
+			spk_reset_default_value("pitch", synth->default_pitch,
+				value);
+			spk_reset_default_value("vol", synth->default_vol,
+				value);
+		}
 		break;
 	case VAR_STRING:
-		len = strlen(buf);
-		if ((len >= 1) && (buf[len - 1] == '\n'))
+		len = strlen(cp);
+		if ((len >= 1) && (cp[len - 1] == '\n'))
 			--len;
-		if ((len >= 2) && (buf[0] == '"') && (buf[len - 1] == '"')) {
-			++buf;
+		if ((len >= 2) && (cp[0] == '"') && (cp[len - 1] == '"')) {
+			++cp;
 			len -= 2;
 		}
-		cp = (char *) buf;
 		cp[len] = '\0';
-		ret = spk_set_string_var(buf, param, len);
+		ret = spk_set_string_var(cp, param, len);
 		if (ret == -E2BIG)
 			pr_warn("value too long for %s\n",
-					attr->attr.name);
+					param->name);
 		break;
 	default:
 		pr_warn("%s unknown type %d\n",
 			param->name, (int)param->var_type);
-	break;
-	}
-	/*
-	 * If voice was just changed, we might need to reset our default
-	 * pitch and volume.
-	 */
-	if (strcmp(attr->attr.name, "voice") == 0) {
-		if (synth && synth->default_pitch) {
-			param = spk_var_header_by_name("pitch");
-			if (param)  {
-				spk_set_num_var(synth->default_pitch[value],
-						param, E_NEW_DEFAULT);
-				spk_set_num_var(0, param, E_DEFAULT);
-			}
-		}
-		if (synth && synth->default_vol) {
-			param = spk_var_header_by_name("vol");
-			if (param)  {
-				spk_set_num_var(synth->default_vol[value],
-						param, E_NEW_DEFAULT);
-				spk_set_num_var(0, param, E_DEFAULT);
-			}
-		}
-	}
+		break;
+	}
 	spk_unlock(flags);
 
 	if (ret == -ERESTART)
-		pr_info("%s reset to default value\n", attr->attr.name);
+		pr_info("%s reset to default value\n", param->name);
 	return count;
 }
 EXPORT_SYMBOL_GPL(spk_var_store);
-- 
1.7.2.5

Re: [PATCH 1/1] staging/speakup/kobjects.c: Code improvement.

[Samuel Thibault]

Raphael S. Carvalho, le Fri 21 Jun 2013 22:54:40 -0300, a écrit :
> Well, there is no need to use strcmp since we can make a test of similar semantic by using the var_id field of param.
> I moved the test into the VAR_NUM:VAR_TIME case since VAR_STRING will never be "voice".
> 
> spk_xlate isn't used anymore (in line 608), then there is no difference between using cp and buf in VAR_STRING case.
> Besides, buf is a const char and those changes remove one uneeded line.
> 
> I created the function spk_reset_default_value because it clarifies the code and allows code reusing.
> 
> Signed-off-by: Raphael S. Carvalho <raphael.scarv@gmail.com>

Acked-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

> ---
>  drivers/staging/speakup/kobjects.c |   73 +++++++++++++++++++----------------
>  1 files changed, 40 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/staging/speakup/kobjects.c b/drivers/staging/speakup/kobjects.c
> index 943b6c1..4660ce3 100644
> --- a/drivers/staging/speakup/kobjects.c
> +++ b/drivers/staging/speakup/kobjects.c
> @@ -585,6 +585,25 @@ ssize_t spk_var_show(struct kobject *kobj, struct kobj_attribute *attr,
>  }
>  EXPORT_SYMBOL_GPL(spk_var_show);
>  
> +/*
> + * Used to reset either default_pitch or default_vol.
> + */
> +static inline void spk_reset_default_value(char *header_name,
> +					int *synth_default_value, int idx)
> +{
> +	struct st_var_header *param;
> +
> +	if (synth && synth_default_value) {
> +		param = spk_var_header_by_name(header_name);
> +		if (param)  {
> +			spk_set_num_var(synth_default_value[idx],
> +					param, E_NEW_DEFAULT);
> +			spk_set_num_var(0, param, E_DEFAULT);
> +			pr_info("%s reset to default value\n", param->name);
> +		}
> +	}
> +}
> +
>  /*
>   * This function is called when a user echos a value to one of the
>   * variable parameters.
> @@ -624,56 +643,44 @@ ssize_t spk_var_store(struct kobject *kobj, struct kobj_attribute *attr,
>  		if (ret == -ERANGE) {
>  			var_data = param->data;
>  			pr_warn("value for %s out of range, expect %d to %d\n",
> -				attr->attr.name,
> +				param->name,
>  				var_data->u.n.low, var_data->u.n.high);
>  		}
> +
> +	       /*
> +		* If voice was just changed, we might need to reset our default
> +		* pitch and volume.
> +		*/
> +		if (param->var_id == VOICE) {
> +			spk_reset_default_value("pitch", synth->default_pitch,
> +				value);
> +			spk_reset_default_value("vol", synth->default_vol,
> +				value);
> +		}
>  		break;
>  	case VAR_STRING:
> -		len = strlen(buf);
> -		if ((len >= 1) && (buf[len - 1] == '\n'))
> +		len = strlen(cp);
> +		if ((len >= 1) && (cp[len - 1] == '\n'))
>  			--len;
> -		if ((len >= 2) && (buf[0] == '"') && (buf[len - 1] == '"')) {
> -			++buf;
> +		if ((len >= 2) && (cp[0] == '"') && (cp[len - 1] == '"')) {
> +			++cp;
>  			len -= 2;
>  		}
> -		cp = (char *) buf;
>  		cp[len] = '\0';
> -		ret = spk_set_string_var(buf, param, len);
> +		ret = spk_set_string_var(cp, param, len);
>  		if (ret == -E2BIG)
>  			pr_warn("value too long for %s\n",
> -					attr->attr.name);
> +					param->name);
>  		break;
>  	default:
>  		pr_warn("%s unknown type %d\n",
>  			param->name, (int)param->var_type);
> -	break;
> -	}
> -	/*
> -	 * If voice was just changed, we might need to reset our default
> -	 * pitch and volume.
> -	 */
> -	if (strcmp(attr->attr.name, "voice") == 0) {
> -		if (synth && synth->default_pitch) {
> -			param = spk_var_header_by_name("pitch");
> -			if (param)  {
> -				spk_set_num_var(synth->default_pitch[value],
> -						param, E_NEW_DEFAULT);
> -				spk_set_num_var(0, param, E_DEFAULT);
> -			}
> -		}
> -		if (synth && synth->default_vol) {
> -			param = spk_var_header_by_name("vol");
> -			if (param)  {
> -				spk_set_num_var(synth->default_vol[value],
> -						param, E_NEW_DEFAULT);
> -				spk_set_num_var(0, param, E_DEFAULT);
> -			}
> -		}
> -	}
> +		break;
> +	}
>  	spk_unlock(flags);
>  
>  	if (ret == -ERESTART)
> -		pr_info("%s reset to default value\n", attr->attr.name);
> +		pr_info("%s reset to default value\n", param->name);
>  	return count;
>  }
>  EXPORT_SYMBOL_GPL(spk_var_store);
> -- 
> 1.7.2.5
> 

-- 
Samuel
$ du temp.iso 
2,0T    temp.iso
$ ls temp.iso -l
-r-xr-xr-x    1 samy     thibault      16E 2003-03-22 14:44 temp.iso*
 -+- je vous dirai pas la marque de mon disque dur, na :p -+- 

Re: [PATCH 1/1] staging/speakup/kobjects.c: Code improvement.

[Greg Kroah-Hartman]

On Fri, Jun 21, 2013 at 10:54:40PM -0300, Raphael S. Carvalho wrote:
> Well, there is no need to use strcmp since we can make a test of similar
> semantic by using the var_id field of param.
> I moved the test into the VAR_NUM:VAR_TIME case since VAR_STRING will
> never be "voice".
> 
> spk_xlate isn't used anymore (in line 608), then there is no difference
> between using cp and buf in VAR_STRING case.
> Besides, buf is a const char and those changes remove one uneeded line.
> 
> I created the function spk_reset_default_value because it clarifies the
> code and allows code reusing.
> 
> Signed-off-by: Raphael S. Carvalho <raphael.scarv@gmail.com>
> Acked-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
> ---
>  drivers/staging/speakup/kobjects.c |   73 +++++++++++++++++++----------------
>  1 files changed, 40 insertions(+), 33 deletions(-)

This patch no longer applies to my tree, can you please refresh it
against linux-next and resend, keeping Samuel's ack on it?

thanks,

greg k-h