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

2013-09-11 Thread 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 

Acked-by: Samuel Thibault 

> ---
>  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);
> - }
> - }
> - }
> + }
>   

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

2013-09-11 Thread 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

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


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

2013-09-11 Thread 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

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


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

2013-09-11 Thread 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.sc...@gmail.com

Acked-by: Samuel Thibault samuel.thiba...@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, 

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

2013-09-10 Thread Chris Brannon
Dan Carpenter  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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2013-09-10 Thread 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

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


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

2013-09-10 Thread 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

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


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

2013-09-10 Thread Chris Brannon
Dan Carpenter dan.carpen...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2013-09-09 Thread Chris Brannon
Raphael S Carvalho  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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2013-09-09 Thread Chris Brannon
Raphael S Carvalho raphael.sc...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2013-09-08 Thread Raphael S Carvalho
On Mon, Sep 9, 2013 at 12:56 AM, Chris Brannon  wrote:
>
> "Raphael S.Carvalho"  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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2013-09-08 Thread Chris Brannon
"Raphael S.Carvalho"  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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2013-09-08 Thread Chris Brannon
Raphael S.Carvalho raphael.sc...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2013-09-08 Thread Raphael S Carvalho
On Mon, Sep 9, 2013 at 12:56 AM, Chris Brannon ch...@the-brannons.com wrote:

 Raphael S.Carvalho raphael.sc...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2013-09-02 Thread 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 
---
 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(_info.spinlock, flags);
 
if (ret == -ERESTART)
-   pr_info("%s reset to default value\n", attr->attr.name);
+   pr_info("%s 

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

2013-09-02 Thread 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.sc...@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);

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

2013-07-23 Thread 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 
> Acked-by: Samuel Thibault 
> ---
>  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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2013-07-23 Thread 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.sc...@gmail.com
 Acked-by: Samuel Thibault samuel.thiba...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2013-06-30 Thread 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 

Acked-by: Samuel Thibault 

> ---
>  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;
> +  

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

2013-06-30 Thread 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.sc...@gmail.com

Acked-by: Samuel Thibault samuel.thiba...@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);
 

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

2013-06-21 Thread 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 
---
 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 

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

2013-06-21 Thread 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.sc...@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