* [PATCH] powercap: Adjust printing the constraint name with new line @ 2020-11-09 17:24 Lukasz Luba 2020-11-10 19:19 ` Rafael J. Wysocki 0 siblings, 1 reply; 5+ messages in thread From: Lukasz Luba @ 2020-11-09 17:24 UTC (permalink / raw) To: linux-kernel, linux-pm, rafael; +Cc: daniel.lezcano, rjw, lukasz.luba 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'; } } -- 2.17.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] powercap: Adjust printing the constraint name with new line 2020-11-09 17:24 [PATCH] powercap: Adjust printing the constraint name with new line Lukasz Luba @ 2020-11-10 19:19 ` Rafael J. Wysocki 2020-11-17 9:22 ` Lukasz Luba 0 siblings, 1 reply; 5+ messages in thread From: Rafael J. Wysocki @ 2020-11-10 19:19 UTC (permalink / raw) To: Lukasz Luba Cc: Linux Kernel Mailing List, Linux PM, Rafael J. Wysocki, Daniel Lezcano, Rafael J. Wysocki 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? > } > } > > -- ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] powercap: Adjust printing the constraint name with new line 2020-11-10 19:19 ` Rafael J. Wysocki @ 2020-11-17 9:22 ` Lukasz Luba 2020-11-17 12:51 ` Rafael J. Wysocki 0 siblings, 1 reply; 5+ messages in thread From: Lukasz Luba @ 2020-11-17 9:22 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Linux Kernel Mailing List, Linux PM, Daniel Lezcano, Rafael J. Wysocki 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] powercap: Adjust printing the constraint name with new line 2020-11-17 9:22 ` Lukasz Luba @ 2020-11-17 12:51 ` Rafael J. Wysocki 2020-11-17 12:57 ` Lukasz Luba 0 siblings, 1 reply; 5+ messages in thread From: Rafael J. Wysocki @ 2020-11-17 12:51 UTC (permalink / raw) To: Lukasz Luba Cc: Rafael J. Wysocki, Linux Kernel Mailing List, Linux PM, Daniel Lezcano, Rafael J. Wysocki 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! ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] powercap: Adjust printing the constraint name with new line 2020-11-17 12:51 ` Rafael J. Wysocki @ 2020-11-17 12:57 ` Lukasz Luba 0 siblings, 0 replies; 5+ messages in thread From: Lukasz Luba @ 2020-11-17 12:57 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Linux Kernel Mailing List, Linux PM, Daniel Lezcano, Rafael J. Wysocki 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-11-17 12:57 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-11-09 17:24 [PATCH] powercap: Adjust printing the constraint name with new line Lukasz Luba 2020-11-10 19:19 ` Rafael J. Wysocki 2020-11-17 9:22 ` Lukasz Luba 2020-11-17 12:51 ` Rafael J. Wysocki 2020-11-17 12:57 ` Lukasz Luba
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®