| Message ID | 20230913092701.440959-2-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [v1,1/2] platform/x86: think-lmi: Replace kstrdup() + strreplace() with kstrdup_and_replace() | expand |
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 <andriy.shevchenko@linux.intel.com> > --- > 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 <andriy.shevchenko@linux.intel.com> > --- > 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 <ilpo.jarvinen@linux.intel.com>
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.
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);
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 <andriy.shevchenko@linux.intel.com> --- drivers/platform/x86/think-lmi.c | 19 ++++--------------- 1 file changed, 4 insertions(+), 15 deletions(-)