getTrigger() functions returns incorrect value

[Kitsok]

Hello!

getTrigger() functions implemented like this: std::string SysfsLed::getTrigger() { return get_sysfs_attr<std::string>(root / TRIGGER); } If LED is in blink state, /sys/class/leds//trigger contains following: none [timer] heartbeat default-on

In this case getTrigger() returns "none" (first word of the line).

This leads to incorrect initial setting of LED. In Physical::setInitialState() the content of "brightness" sysfs attribute is read if trigger is not "timer" (and it's not as it's "none"). But when LEDs is blinking, the read of brightness is undefined as it may read 0 or 1.

[amboar]

Hi @Kitsok, thanks for the bug report. I'll look into it.

[Kitsok]

Hi!

Here is how I've fixed it:

diff --git a/sysfs.cpp b/sysfs.cpp
index 985a991..4893c56 100644
--- a/sysfs.cpp
+++ b/sysfs.cpp
@@ -19,6 +19,7 @@
 #include <fstream>
 #include <iostream>
 #include <string>
+#include <boost/regex.hpp>

 namespace fs = std::filesystem;

@@ -41,7 +42,7 @@ std::string get_sysfs_attr(const fs::path& path)
 {   
     std::string content;
     std::ifstream file(path);
-    file >> content;
+    std::getline(file, content);
     return content;
 }

@@ -76,7 +77,16 @@ unsigned long SysfsLed::getMaxBrightness()

 std::string SysfsLed::getTrigger()
 {
-    return get_sysfs_attr<std::string>(root / TRIGGER);
+    std::string trigger_line = get_sysfs_attr<std::string>(root / TRIGGER);
+
+    boost::regex exp(".*\\[(\\w+)\\].*", boost::regex::extended);
+    boost::smatch match;
+
+    if (boost::regex_search(trigger_line, match, exp))
+    {
+        return std::string(match[1].first, match[1].second);
+    }
+    return "none";
 }

 void SysfsLed::setTrigger(const std::string& trigger)

[williamspatrick]

Here is how I've fixed it:

Can you commit this in Gerrit? Are you part of a company who has signed a CLA already?

[williamspatrick]

• boost::regex exp(".\[(\w+)\].", boost::regex::extended);

fwiw, I think regex is pretty heavy for this situation. substr(find_first_of('['), find_first_of(']')) is probably sufficient (with a little bounds checking added).

[Kitsok]

Here is how I've fixed it:

Can you commit this in Gerrit? Are you part of a company who has signed a CLA already?

I have account on Gerrit, but don't have signed CLA, that's why I don't commit directly

[Kitsok]

fwiw, I think regex is pretty heavy for this situation

Agree

diff --git a/sysfs.cpp b/sysfs.cpp
index 985a991..a44b3e0 100644
--- a/sysfs.cpp
+++ b/sysfs.cpp
@@ -41,7 +41,7 @@ std::string get_sysfs_attr(const fs::path& path)
 {   
     std::string content;
     std::ifstream file(path);
-    file >> content;
+    std::getline(file, content);
     return content;
 }

@@ -76,7 +76,22 @@ unsigned long SysfsLed::getMaxBrightness()

 std::string SysfsLed::getTrigger()
 {
-    return get_sysfs_attr<std::string>(root / TRIGGER);
+    std::string trigger_line = get_sysfs_attr<std::string>(root / TRIGGER);
+
+    size_t start = trigger_line.find_first_of('[');
+    size_t end = trigger_line.find_first_of(']');
+    if (start >= end ||
+        start == std::string::npos ||
+        end == std::string::npos)
+    {
+        return "none";
+    }
+
+    std::string rc = trigger_line.substr(start + 1, end - start - 1);
+    if (rc == "")
+        return "none";
+
+    return rc;
 }

 void SysfsLed::setTrigger(const std::string& trigger)

[lxwinspur]

@Kitsok We had the same problem, I checked the patch and there was no upstream, using this patch solved the problem. Forgive me for upstreaming this patch.

Review by: https://gerrit.openbmc.org/c/openbmc/phosphor-led-sysfs/+/68041

[Kitsok]

Hello! No problem at all, I'm glad the patch helped. 01.12.2023, 10:44, "George Liu" @.>@. had the same problem, I checked the patch and there was no upstream, using this patch solved the problem.Forgive me for upstreaming this patch.Review by: https://gerrit.openbmc.org/c/openbmc/phosphor-led-sysfs/+/68041—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>  -- Best regards,Konstantin Klubnichkin,lead firmware engineer,server hardware R&D group,Yandex Europe B.V.tel: +31-684-515-740