Re: [PATCH] powercap: Adjust printing the constraint name with new line

From: Lukasz Luba
Date: Tue Nov 17 2020 - 07:57:57 EST




On 11/17/20 12:51 PM, Rafael J. Wysocki wrote:
On Tue, Nov 17, 2020 at 10:22 AM Lukasz Luba <lukasz.luba@xxxxxxx> 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@xxxxxxx> 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@xxxxxxx>
---
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