Re: [PATCH 1/1] staging/speakup/kobjects.c: Code improvement.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
"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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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