* [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 related [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.