We can replace
p = strchrnul(str, '$OLD');
*p = '\0';
with
strreplace(str, '$OLD', '\0');
that does the compatible modification without a need of the temporary variable.
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/platform/x86/think-lmi.c | 19 ++++---------------
1 file changed, 4 insertions(+), 15 deletions(-)
diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
index 94a3c7a74bc4..2f20fafe7f55 100644
--- a/drivers/platform/x86/think-lmi.c
+++ b/drivers/platform/x86/think-lmi.c
@@ -198,14 +198,6 @@ static struct think_lmi tlmi_priv;
static struct class *fw_attr_class;
static DEFINE_MUTEX(tlmi_mutex);
-/* ------ Utility functions ------------*/
-/* Strip out CR if one is present */
-static void strip_cr(char *str)
-{
- char *p = strchrnul(str, '\n');
- *p = '\0';
-}
-
/* Convert BIOS WMI error string to suitable error code */
static int tlmi_errstr_to_err(const char *errstr)
{
@@ -411,7 +403,7 @@ static ssize_t current_password_store(struct kobject *kobj,
strscpy(setting->password, buf, setting->maxlen);
/* Strip out CR if one is present, setting password won't work if it is present */
- strip_cr(setting->password);
+ strreplace(setting->password, '\n', '\0');
return count;
}
@@ -921,7 +913,7 @@ static ssize_t display_name_show(struct kobject *kobj, struct kobj_attribute *at
static ssize_t current_value_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
{
struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);
- char *item, *value, *p;
+ char *item, *value;
int ret;
ret = tlmi_setting(setting->index, &item, LENOVO_BIOS_SETTING_GUID);
@@ -934,8 +926,7 @@ static ssize_t current_value_show(struct kobject *kobj, struct kobj_attribute *a
ret = -EINVAL;
else {
/* On Workstations remove the Options part after the value */
- p = strchrnul(value, ';');
- *p = '\0';
+ strreplace(value, ';', '\0');
ret = sysfs_emit(buf, "%s\n", value + 1);
}
kfree(item);
@@ -1418,7 +1409,6 @@ static int tlmi_analyze(void)
for (i = 0; i < TLMI_SETTINGS_COUNT; ++i) {
struct tlmi_attr_setting *setting;
char *item = NULL;
- char *p;
tlmi_priv.setting[i] = NULL;
ret = tlmi_setting(i, &item, LENOVO_BIOS_SETTING_GUID);
@@ -1435,8 +1425,7 @@ static int tlmi_analyze(void)
strreplace(item, '/', '\\');
/* Remove the value part */
- p = strchrnul(item, ',');
- *p = '\0';
+ strreplace(item, ',', '\0');
/* Create a setting entry */
setting = kzalloc(sizeof(*setting), GFP_KERNEL);
--
2.40.0.1.gaa8946217a0b
On Wed, Sep 13, 2023, at 5:27 AM, Andy Shevchenko wrote:
> We can replace
> p = strchrnul(str, '$OLD');
> *p = '\0';
> with
> strreplace(str, '$OLD', '\0');
> that does the compatible modification without a need of the temporary variable.
>
> Signed-off-by: Andy Shevchenko <[email protected]>
> ---
> drivers/platform/x86/think-lmi.c | 19 ++++---------------
> 1 file changed, 4 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
> index 94a3c7a74bc4..2f20fafe7f55 100644
> --- a/drivers/platform/x86/think-lmi.c
> +++ b/drivers/platform/x86/think-lmi.c
> @@ -198,14 +198,6 @@ static struct think_lmi tlmi_priv;
> static struct class *fw_attr_class;
> static DEFINE_MUTEX(tlmi_mutex);
>
> -/* ------ Utility functions ------------*/
> -/* Strip out CR if one is present */
> -static void strip_cr(char *str)
> -{
> - char *p = strchrnul(str, '\n');
> - *p = '\0';
> -}
> -
> /* Convert BIOS WMI error string to suitable error code */
> static int tlmi_errstr_to_err(const char *errstr)
> {
> @@ -411,7 +403,7 @@ static ssize_t current_password_store(struct kobject *kobj,
>
> strscpy(setting->password, buf, setting->maxlen);
> /* Strip out CR if one is present, setting password won't work if it
> is present */
> - strip_cr(setting->password);
> + strreplace(setting->password, '\n', '\0');
> return count;
> }
>
> @@ -921,7 +913,7 @@ static ssize_t display_name_show(struct kobject
> *kobj, struct kobj_attribute *at
> static ssize_t current_value_show(struct kobject *kobj, struct
> kobj_attribute *attr, char *buf)
> {
> struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);
> - char *item, *value, *p;
> + char *item, *value;
> int ret;
>
> ret = tlmi_setting(setting->index, &item, LENOVO_BIOS_SETTING_GUID);
> @@ -934,8 +926,7 @@ static ssize_t current_value_show(struct kobject
> *kobj, struct kobj_attribute *a
> ret = -EINVAL;
> else {
> /* On Workstations remove the Options part after the value */
> - p = strchrnul(value, ';');
> - *p = '\0';
> + strreplace(value, ';', '\0');
> ret = sysfs_emit(buf, "%s\n", value + 1);
> }
> kfree(item);
> @@ -1418,7 +1409,6 @@ static int tlmi_analyze(void)
> for (i = 0; i < TLMI_SETTINGS_COUNT; ++i) {
> struct tlmi_attr_setting *setting;
> char *item = NULL;
> - char *p;
>
> tlmi_priv.setting[i] = NULL;
> ret = tlmi_setting(i, &item, LENOVO_BIOS_SETTING_GUID);
> @@ -1435,8 +1425,7 @@ static int tlmi_analyze(void)
> strreplace(item, '/', '\\');
>
> /* Remove the value part */
> - p = strchrnul(item, ',');
> - *p = '\0';
> + strreplace(item, ',', '\0');
>
> /* Create a setting entry */
> setting = kzalloc(sizeof(*setting), GFP_KERNEL);
> --
> 2.40.0.1.gaa8946217a0b
Looks good to me. Will aim to test and confirm on some systems.
Thanks for the cleanup!
Mark
On Wed, 13 Sep 2023, Andy Shevchenko wrote:
> We can replace
> p = strchrnul(str, '$OLD');
> *p = '\0';
> with
> strreplace(str, '$OLD', '\0');
> that does the compatible modification without a need of the temporary variable.
>
> Signed-off-by: Andy Shevchenko <[email protected]>
> ---
> drivers/platform/x86/think-lmi.c | 19 ++++---------------
> 1 file changed, 4 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
> index 94a3c7a74bc4..2f20fafe7f55 100644
> --- a/drivers/platform/x86/think-lmi.c
> +++ b/drivers/platform/x86/think-lmi.c
> @@ -198,14 +198,6 @@ static struct think_lmi tlmi_priv;
> static struct class *fw_attr_class;
> static DEFINE_MUTEX(tlmi_mutex);
>
> -/* ------ Utility functions ------------*/
> -/* Strip out CR if one is present */
> -static void strip_cr(char *str)
> -{
> - char *p = strchrnul(str, '\n');
> - *p = '\0';
> -}
> -
> /* Convert BIOS WMI error string to suitable error code */
> static int tlmi_errstr_to_err(const char *errstr)
> {
> @@ -411,7 +403,7 @@ static ssize_t current_password_store(struct kobject *kobj,
>
> strscpy(setting->password, buf, setting->maxlen);
> /* Strip out CR if one is present, setting password won't work if it is present */
> - strip_cr(setting->password);
> + strreplace(setting->password, '\n', '\0');
> return count;
> }
>
> @@ -921,7 +913,7 @@ static ssize_t display_name_show(struct kobject *kobj, struct kobj_attribute *at
> static ssize_t current_value_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> {
> struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);
> - char *item, *value, *p;
> + char *item, *value;
> int ret;
>
> ret = tlmi_setting(setting->index, &item, LENOVO_BIOS_SETTING_GUID);
> @@ -934,8 +926,7 @@ static ssize_t current_value_show(struct kobject *kobj, struct kobj_attribute *a
> ret = -EINVAL;
> else {
> /* On Workstations remove the Options part after the value */
> - p = strchrnul(value, ';');
> - *p = '\0';
> + strreplace(value, ';', '\0');
> ret = sysfs_emit(buf, "%s\n", value + 1);
> }
> kfree(item);
> @@ -1418,7 +1409,6 @@ static int tlmi_analyze(void)
> for (i = 0; i < TLMI_SETTINGS_COUNT; ++i) {
> struct tlmi_attr_setting *setting;
> char *item = NULL;
> - char *p;
>
> tlmi_priv.setting[i] = NULL;
> ret = tlmi_setting(i, &item, LENOVO_BIOS_SETTING_GUID);
> @@ -1435,8 +1425,7 @@ static int tlmi_analyze(void)
> strreplace(item, '/', '\\');
>
> /* Remove the value part */
> - p = strchrnul(item, ',');
> - *p = '\0';
> + strreplace(item, ',', '\0');
>
> /* Create a setting entry */
> setting = kzalloc(sizeof(*setting), GFP_KERNEL);
>
Reviewed-by: Ilpo J?rvinen <[email protected]>
--
i.
On 13/09/2023 11.27, Andy Shevchenko wrote:
> @@ -921,7 +913,7 @@ static ssize_t display_name_show(struct kobject *kobj, struct kobj_attribute *at
> static ssize_t current_value_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> {
> struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);
> - char *item, *value, *p;
> + char *item, *value;
> int ret;
>
> ret = tlmi_setting(setting->index, &item, LENOVO_BIOS_SETTING_GUID);
> @@ -934,8 +926,7 @@ static ssize_t current_value_show(struct kobject *kobj, struct kobj_attribute *a
> ret = -EINVAL;
> else {
> /* On Workstations remove the Options part after the value */
> - p = strchrnul(value, ';');
> - *p = '\0';
> + strreplace(value, ';', '\0');
So how do you know that the string contains at most one ';'? Same for
all the other replacements. If that's not guaranteed, this is not at all
equivalent.
Or maybe the result is just used a normal string afterwards, and it
doesn't matter at all how the content after the first ';' has been mangled?
It's certainly not obvious to me that this is correct, but of course I
know nothing about this code.
Rasmus
On Mon, Sep 18, 2023 at 04:48:40PM +0200, Rasmus Villemoes wrote:
> On 13/09/2023 11.27, Andy Shevchenko wrote:
> > - p = strchrnul(value, ';');
> > - *p = '\0';
> > + strreplace(value, ';', '\0');
>
> So how do you know that the string contains at most one ';'? Same for
> all the other replacements. If that's not guaranteed, this is not at all
> equivalent.
>
> Or maybe the result is just used a normal string afterwards, and it
> doesn't matter at all how the content after the first ';' has been mangled?
>
> It's certainly not obvious to me that this is correct, but of course I
> know nothing about this code.
If you read the comment and code slightly above you may get that this is not
a problem at all. There are no side effects as the part after first occurrence
of ; is not used and original string is NUL-terminated.
--
With Best Regards,
Andy Shevchenko