| Message ID | 20241108232438.269156-2-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | power: supply: Add new "charge_types" property | expand |
On Sat, Nov 9, 2024 at 1:24 AM Hans de Goede <hdegoede@redhat.com> wrote: > > Some enum style power-supply properties have text-values / labels for some > of the enum values containing a space, e.g. "Long Life" for > POWER_SUPPLY_CHARGE_TYPE_LONGLIFE. > > Make power_supply_show_enum_with_available() replace these spaces with > '_' when showing the available text-values. After this the output for > a battery which supports "Long Life" will be e.g.: > > Fast [Standard] Long_Life > > or: > > Fast Standard [Long_Life] > > Modify power_supply_store_property() to accept both the original text-value > with space and the alternative value with the spaces replaced by '_'. > This allows users to write the value with '_' after seeing this on reading > the property. ... > +static void power_supply_escape_spaces(const char *str, char *buf, size_t bufsize) > +{ > + strscpy(buf, str, bufsize); > + strreplace(buf, ' ', '_'); > +} The bufsize in all cases here is sizeof(buf), making the above to be a macro we may switch to 2-argument strscpy(). FTR, it embeds the check that buf is an array. ... > + char escaped_label[32]; Even more, the but size seems also the same, can we have buf defined inside the above?
Hi, On 9-Nov-24 5:30 AM, Andy Shevchenko wrote: > On Sat, Nov 9, 2024 at 1:24 AM Hans de Goede <hdegoede@redhat.com> wrote: >> >> Some enum style power-supply properties have text-values / labels for some >> of the enum values containing a space, e.g. "Long Life" for >> POWER_SUPPLY_CHARGE_TYPE_LONGLIFE. >> >> Make power_supply_show_enum_with_available() replace these spaces with >> '_' when showing the available text-values. After this the output for >> a battery which supports "Long Life" will be e.g.: >> >> Fast [Standard] Long_Life >> >> or: >> >> Fast Standard [Long_Life] >> >> Modify power_supply_store_property() to accept both the original text-value >> with space and the alternative value with the spaces replaced by '_'. >> This allows users to write the value with '_' after seeing this on reading >> the property. > > ... > >> +static void power_supply_escape_spaces(const char *str, char *buf, size_t bufsize) >> +{ >> + strscpy(buf, str, bufsize); >> + strreplace(buf, ' ', '_'); >> +} > > The bufsize in all cases here is sizeof(buf), making the above to be a > macro we may switch to 2-argument strscpy(). FTR, it embeds the check > that buf is an array. I did think about this already, but using a macro makes this harder to read just to save 2 sizeof() calls. So I prefer doing things this way. > > ... > >> + char escaped_label[32]; > > Even more, the but size seems also the same, can we have buf defined > inside the above? No not really, its address would need to be returned then, requiring it to be static, at which point we get race conditions when multiple threads use the same function at the same time. Regards, Hans
On Sat, Nov 9, 2024 at 12:52 PM Hans de Goede <hdegoede@redhat.com> wrote: > On 9-Nov-24 5:30 AM, Andy Shevchenko wrote: > > On Sat, Nov 9, 2024 at 1:24 AM Hans de Goede <hdegoede@redhat.com> wrote: ... > >> +static void power_supply_escape_spaces(const char *str, char *buf, size_t bufsize) > >> +{ > >> + strscpy(buf, str, bufsize); > >> + strreplace(buf, ' ', '_'); > >> +} > > > > The bufsize in all cases here is sizeof(buf), making the above to be a > > macro we may switch to 2-argument strscpy(). FTR, it embeds the check > > that buf is an array. > > I did think about this already, but using a macro makes this harder > to read just to save 2 sizeof() calls. So I prefer doing things > this way. ... > >> + char escaped_label[32]; > > > > Even more, the but size seems also the same, can we have buf defined > > inside the above? > > No not really, its address would need to be returned then, requiring > it to be static, at which point we get race conditions when multiple > threads use the same function at the same time. I meant a macro case, but it's not a big deal after all. Perhaps we can evolve a macro in the generic headers if there are enough users, like #define str_copy_and_replace(...) ...
diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c index 16b3c5880cd8..6f128c6fa62f 100644 --- a/drivers/power/supply/power_supply_sysfs.c +++ b/drivers/power/supply/power_supply_sysfs.c @@ -237,23 +237,52 @@ static enum power_supply_property dev_attr_psp(struct device_attribute *attr) return to_ps_attr(attr) - power_supply_attrs; } +static void power_supply_escape_spaces(const char *str, char *buf, size_t bufsize) +{ + strscpy(buf, str, bufsize); + strreplace(buf, ' ', '_'); +} + +static int power_supply_match_string(const char * const *array, size_t n, const char *s) +{ + int ret; + + /* First try an exact match */ + ret = __sysfs_match_string(array, n, s); + if (ret >= 0) + return ret; + + /* Second round, try matching with spaces replaced by '_' */ + for (size_t i = 0; i < n; i++) { + char buf[32]; + + power_supply_escape_spaces(array[i], buf, sizeof(buf)); + if (sysfs_streq(buf, s)) + return i; + } + + return -EINVAL; +} + static ssize_t power_supply_show_enum_with_available( struct device *dev, const char * const labels[], int label_count, unsigned int available_values, int value, char *buf) { bool match = false, available, active; + char escaped_label[32]; ssize_t count = 0; int i; for (i = 0; i < label_count; i++) { available = available_values & BIT(i); active = i == value; + power_supply_escape_spaces(labels[i], escaped_label, sizeof(escaped_label)); if (available && active) { - count += sysfs_emit_at(buf, count, "[%s] ", labels[i]); + count += sysfs_emit_at(buf, count, "[%s] ", escaped_label); match = true; } else if (available) { - count += sysfs_emit_at(buf, count, "%s ", labels[i]); + count += sysfs_emit_at(buf, count, "%s ", escaped_label); } } @@ -332,8 +361,8 @@ static ssize_t power_supply_store_property(struct device *dev, ret = -EINVAL; if (ps_attr->text_values_len > 0) { - ret = __sysfs_match_string(ps_attr->text_values, - ps_attr->text_values_len, buf); + ret = power_supply_match_string(ps_attr->text_values, + ps_attr->text_values_len, buf); } /*
Some enum style power-supply properties have text-values / labels for some of the enum values containing a space, e.g. "Long Life" for POWER_SUPPLY_CHARGE_TYPE_LONGLIFE. Make power_supply_show_enum_with_available() replace these spaces with '_' when showing the available text-values. After this the output for a battery which supports "Long Life" will be e.g.: Fast [Standard] Long_Life or: Fast Standard [Long_Life] Modify power_supply_store_property() to accept both the original text-value with space and the alternative value with the spaces replaced by '_'. This allows users to write the value with '_' after seeing this on reading the property. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- Changes in v2 - Replace spaces with '_' instead of surrounding the text-value by "" --- drivers/power/supply/power_supply_sysfs.c | 37 ++++++++++++++++++++--- 1 file changed, 33 insertions(+), 4 deletions(-)