| Message ID | 20201109172452.6923-1-lukasz.luba@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | powercap: Adjust printing the constraint name with new line | expand |
On Mon, Nov 9, 2020 at 6:25 PM Lukasz Luba <lukasz.luba@arm.com> wrote: > > The constraint name has limit of size 30, which sometimes might be hit. > When this happens the new line might be lost. Prevent this and set the > new line when the name string is too long." > > Signed-off-by: Lukasz Luba <lukasz.luba@arm.com> > --- > drivers/powercap/powercap_sys.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/powercap/powercap_sys.c b/drivers/powercap/powercap_sys.c > index f808c5fa9838..575f9fdb810e 100644 > --- a/drivers/powercap/powercap_sys.c > +++ b/drivers/powercap/powercap_sys.c > @@ -174,6 +174,10 @@ static ssize_t show_constraint_name(struct device *dev, > "%s\n", name); > buf[POWERCAP_CONSTRAINT_NAME_LEN] = '\0'; > len = strlen(buf); > + > + /* When the 'name' is too long, don't lose new line */ > + if (strlen(name) >= POWERCAP_CONSTRAINT_NAME_LEN) > + buf[POWERCAP_CONSTRAINT_NAME_LEN - 1] = '\n'; Wouldn't it be better to pass POWERCAP_CONSTRAINT_NAME_LEN - 1 to snprintf() above? > } > } > > --
Hi Rafael, On 11/10/20 7:19 PM, Rafael J. Wysocki wrote: > On Mon, Nov 9, 2020 at 6:25 PM Lukasz Luba <lukasz.luba@arm.com> wrote: >> >> The constraint name has limit of size 30, which sometimes might be hit. >> When this happens the new line might be lost. Prevent this and set the >> new line when the name string is too long." >> >> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com> >> --- >> drivers/powercap/powercap_sys.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/drivers/powercap/powercap_sys.c b/drivers/powercap/powercap_sys.c >> index f808c5fa9838..575f9fdb810e 100644 >> --- a/drivers/powercap/powercap_sys.c >> +++ b/drivers/powercap/powercap_sys.c >> @@ -174,6 +174,10 @@ static ssize_t show_constraint_name(struct device *dev, >> "%s\n", name); >> buf[POWERCAP_CONSTRAINT_NAME_LEN] = '\0'; >> len = strlen(buf); >> + >> + /* When the 'name' is too long, don't lose new line */ >> + if (strlen(name) >= POWERCAP_CONSTRAINT_NAME_LEN) >> + buf[POWERCAP_CONSTRAINT_NAME_LEN - 1] = '\n'; > > Wouldn't it be better to pass POWERCAP_CONSTRAINT_NAME_LEN - 1 to > snprintf() above? My apologies for delay, I was on holidays. The snprintf() overwrites the '\n' in that case also. I've experimented with it a bit more and tried to come with a bit 'cleaner' solution. What we are looking is probably: "%.*s\n" semantic. Another solution would be: ------------------------------8<--------------------------- if (name) { - snprintf(buf, POWERCAP_CONSTRAINT_NAME_LEN, - "%s\n", name); - buf[POWERCAP_CONSTRAINT_NAME_LEN] = '\0'; + sprintf(buf, "%.*s\n", POWERCAP_CONSTRAINT_NAME_LEN -1, + name); len = strlen(buf); } ----------------------------->8---------------------------- I've check this and it behaves very well. It looks cleaner and it is a used pattern in the kernel. What do you think? Is it good for v2? Regards, Lukasz
On Tue, Nov 17, 2020 at 10:22 AM Lukasz Luba <lukasz.luba@arm.com> wrote: > > Hi Rafael, > > On 11/10/20 7:19 PM, Rafael J. Wysocki wrote: > > On Mon, Nov 9, 2020 at 6:25 PM Lukasz Luba <lukasz.luba@arm.com> wrote: > >> > >> The constraint name has limit of size 30, which sometimes might be hit. > >> When this happens the new line might be lost. Prevent this and set the > >> new line when the name string is too long." > >> > >> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com> > >> --- > >> drivers/powercap/powercap_sys.c | 4 ++++ > >> 1 file changed, 4 insertions(+) > >> > >> diff --git a/drivers/powercap/powercap_sys.c b/drivers/powercap/powercap_sys.c > >> index f808c5fa9838..575f9fdb810e 100644 > >> --- a/drivers/powercap/powercap_sys.c > >> +++ b/drivers/powercap/powercap_sys.c > >> @@ -174,6 +174,10 @@ static ssize_t show_constraint_name(struct device *dev, > >> "%s\n", name); > >> buf[POWERCAP_CONSTRAINT_NAME_LEN] = '\0'; > >> len = strlen(buf); > >> + > >> + /* When the 'name' is too long, don't lose new line */ > >> + if (strlen(name) >= POWERCAP_CONSTRAINT_NAME_LEN) > >> + buf[POWERCAP_CONSTRAINT_NAME_LEN - 1] = '\n'; > > > > Wouldn't it be better to pass POWERCAP_CONSTRAINT_NAME_LEN - 1 to > > snprintf() above? > > My apologies for delay, I was on holidays. > > The snprintf() overwrites the '\n' in that case also. The snprintf() doesn't do that AFAICS, but the next assignment overwrites the newline with the zero character. Anyway, it should have been POWERCAP_CONSTRAINT_NAME_LEN - 2, because it needs to leave space for 2 characters, the newline and the terminating zero. > I've experimented with it a bit more and tried to come with a bit 'cleaner' solution. > > What we are looking is probably: "%.*s\n" semantic. > Another solution would be: > ------------------------------8<--------------------------- > if (name) { > - snprintf(buf, POWERCAP_CONSTRAINT_NAME_LEN, > - "%s\n", > name); > - buf[POWERCAP_CONSTRAINT_NAME_LEN] = '\0'; > + sprintf(buf, "%.*s\n", > POWERCAP_CONSTRAINT_NAME_LEN -1, > + name); > len = strlen(buf); > } > > ----------------------------->8---------------------------- > > I've check this and it behaves very well. > > It looks cleaner and it is a used pattern in the kernel. > What do you think? Is it good for v2? This works for me too. Thanks!
On 11/17/20 12:51 PM, Rafael J. Wysocki wrote: > On Tue, Nov 17, 2020 at 10:22 AM Lukasz Luba <lukasz.luba@arm.com> wrote: >> >> Hi Rafael, >> >> On 11/10/20 7:19 PM, Rafael J. Wysocki wrote: >>> On Mon, Nov 9, 2020 at 6:25 PM Lukasz Luba <lukasz.luba@arm.com> wrote: >>>> >>>> The constraint name has limit of size 30, which sometimes might be hit. >>>> When this happens the new line might be lost. Prevent this and set the >>>> new line when the name string is too long." >>>> >>>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com> >>>> --- >>>> drivers/powercap/powercap_sys.c | 4 ++++ >>>> 1 file changed, 4 insertions(+) >>>> >>>> diff --git a/drivers/powercap/powercap_sys.c b/drivers/powercap/powercap_sys.c >>>> index f808c5fa9838..575f9fdb810e 100644 >>>> --- a/drivers/powercap/powercap_sys.c >>>> +++ b/drivers/powercap/powercap_sys.c >>>> @@ -174,6 +174,10 @@ static ssize_t show_constraint_name(struct device *dev, >>>> "%s\n", name); >>>> buf[POWERCAP_CONSTRAINT_NAME_LEN] = '\0'; >>>> len = strlen(buf); >>>> + >>>> + /* When the 'name' is too long, don't lose new line */ >>>> + if (strlen(name) >= POWERCAP_CONSTRAINT_NAME_LEN) >>>> + buf[POWERCAP_CONSTRAINT_NAME_LEN - 1] = '\n'; >>> >>> Wouldn't it be better to pass POWERCAP_CONSTRAINT_NAME_LEN - 1 to >>> snprintf() above? >> >> My apologies for delay, I was on holidays. >> >> The snprintf() overwrites the '\n' in that case also. > > The snprintf() doesn't do that AFAICS, but the next assignment > overwrites the newline with the zero character. > > Anyway, it should have been POWERCAP_CONSTRAINT_NAME_LEN - 2, because > it needs to leave space for 2 characters, the newline and the > terminating zero. > >> I've experimented with it a bit more and tried to come with a bit 'cleaner' solution. >> >> What we are looking is probably: "%.*s\n" semantic. >> Another solution would be: >> ------------------------------8<--------------------------- >> if (name) { >> - snprintf(buf, POWERCAP_CONSTRAINT_NAME_LEN, >> - "%s\n", >> name); >> - buf[POWERCAP_CONSTRAINT_NAME_LEN] = '\0'; >> + sprintf(buf, "%.*s\n", >> POWERCAP_CONSTRAINT_NAME_LEN -1, >> + name); >> len = strlen(buf); >> } >> >> ----------------------------->8---------------------------- >> >> I've check this and it behaves very well. >> >> It looks cleaner and it is a used pattern in the kernel. >> What do you think? Is it good for v2? > > This works for me too. > > Thanks! > Thank you for the comments. I will send the v2 then. Regards, Lukasz
diff --git a/drivers/powercap/powercap_sys.c b/drivers/powercap/powercap_sys.c index f808c5fa9838..575f9fdb810e 100644 --- a/drivers/powercap/powercap_sys.c +++ b/drivers/powercap/powercap_sys.c @@ -174,6 +174,10 @@ static ssize_t show_constraint_name(struct device *dev, "%s\n", name); buf[POWERCAP_CONSTRAINT_NAME_LEN] = '\0'; len = strlen(buf); + + /* When the 'name' is too long, don't lose new line */ + if (strlen(name) >= POWERCAP_CONSTRAINT_NAME_LEN) + buf[POWERCAP_CONSTRAINT_NAME_LEN - 1] = '\n'; } }
The constraint name has limit of size 30, which sometimes might be hit. When this happens the new line might be lost. Prevent this and set the new line when the name string is too long." Signed-off-by: Lukasz Luba <lukasz.luba@arm.com> --- drivers/powercap/powercap_sys.c | 4 ++++ 1 file changed, 4 insertions(+)