Re: [PATCH 2/4] drivers core: Remove strcat uses around sysfs_emit and neaten

Joe Perches
2020-09-08T08:40:26Z

On Tue, 2020-09-08 at 10:32 +0200, Greg Kroah-Hartman wrote:
> On Mon, Sep 07, 2020 at 10:58:06AM -0700, Joe Perches wrote:
> > strcat is no longer necessary for sysfs_emit and sysfs_emit_at uses.
> >
> > Convert the strcat uses to sysfs_emit calls and neaten other block
> > uses of direct returns to use an intermediate const char *.
[]
> This function is now longer, with an assignment that is not needed
> (type=NULL), so why make this "cleanup"?

It's smaller object code.

[]
> > Again, not a type, it's a state. And you did not merge all sysfs_emit()
> calls into one here, so it's messier now, don't you think?

You can't because the default type uses an argument and not
a fixed string. I don't think it's messier, no.

> > int memory_notify(unsigned long val, void *v)
> > @@ -307,17 +305,16 @@ static ssize_t phys_device_show(struct device *dev,
> > }
> >
> > #ifdef CONFIG_MEMORY_HOTREMOVE
> > -static void print_allowed_zone(char *buf, int nid, unsigned long start_pfn,
> > - unsigned long nr_pages, int online_type,
> > - struct zone *default_zone)
> > +static int print_allowed_zone(char *buf, int len, int nid,
> > + unsigned long start_pfn, unsigned long nr_pages,
> > + int online_type, struct zone *default_zone)
>
> Unrelated change :(

No it's not, it's outputting to buf so it
needs len to output to appropriate spot to
be able to use sysfs_emit_at.

> > {
> > struct zone *zone;
> >
> > zone = zone_for_pfn_range(online_type, nid, start_pfn, nr_pages);
> > - if (zone != default_zone) {
> > - strcat(buf, " ");
> > - strcat(buf, zone->name);
> > - }
> > + if (zone == default_zone)
> > + return 0;
> > + return sysfs_emit_at(buf, len, " %s", zone->name);

here.

> []

> This is better.

All of it is better. <smile>

> > diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
[]
> > @@ -255,9 +255,9 @@ static ssize_t pm_qos_latency_tolerance_us_show(struct device *dev,
> > s32 value = dev_pm_qos_get_user_latency_tolerance(dev);
> >
> > if (value < 0)
> > - return sysfs_emit(buf, "auto\n");
> > + return sysfs_emit(buf, "%s\n", "auto");
> > if (value == PM_QOS_LATENCY_ANY)
> > - return sysfs_emit(buf, "any\n");
> > + return sysfs_emit(buf, "%s\n", "any");
> >
> > return sysfs_emit(buf, "%d\n", value);
> > }
>
> Unrelated change :(

Again, no it's not unrelated, it's consistent.