SysfsUtils: cleanup and improve interface according to the documentation #16097

lrusak · 2019-05-08T02:40:52Z

I'm splitting this out from #14970 as I need it for some other stuff. I have addressed the comments left on that PR here but this can probably use a fresh look over.

as written in the kernel docs:

sysfs allocates a buffer of size (PAGE_SIZE) and passes it to the
method. Sysfs will call the method exactly once for each read or
write.

When writing sysfs files, userspace processes should first read the
entire file, modify the values it wishes to change, then write the
entire buffer back.

for more info please consult the kernel documentation:

https://www.kernel.org/doc/Documentation/filesystems/sysfs.txt

lrusak · 2019-05-11T16:36:01Z

@pkerling mind taking a look at this? I redid the interface in a more c++ manner and it seems to work in my testing. I also added a test case (which I think can be improved).

xbmc/utils/AMLUtils.cpp

xbmc/utils/SysfsUtils.cpp

yol

+1 for the interface changes and adding tests!

Tests for the failure path would also be cool, I guess

xbmc/cores/VideoPlayer/DVDCodecs/Video/AMLCodec.cpp

xbmc/utils/test/TestSysfsUtils.cpp

lrusak · 2019-05-16T02:13:26Z

anyone know what the android build error is about?

Rechi · 2019-05-16T08:58:55Z

fstream can't be used on Android with an API level lower then 24 (current one is 21).

lrusak · 2019-05-16T16:29:33Z

fstream can't be used on Android with an API level lower then 24 (current one is 21).

That seems weird. So the android ndk isn't c++11 compatible?

Not a big deal as we should hopefully drop amlogic support in #16043 which (I think) should remove the sysfs dependency for android

stefansaraev · 2019-05-16T16:57:55Z

That seems weird. So the android ndk isn't c++11 compatible?

it has always been like that. you should bump to api24.

Not a big deal as we should hopefully drop amlogic support in #16043 which (I think) should remove the sysfs dependency for

that won't help at all. aml is linux only.

lrusak · 2019-05-16T17:04:41Z

That seems weird. So the android ndk isn't c++11 compatible?

it has always been like that. you should bump to api24.

Not a big deal as we should hopefully drop amlogic support in #16043 which (I think) should remove the sysfs dependency for

that won't help at all. aml is linux only.

sysfsutils is used in amlutils which is used in the android audio sink

xbmc/utils/SysfsUtils.cpp

xbmc/utils/test/TestSysfsUtils.cpp

MilhouseVH · 2019-11-15T08:14:41Z

Small cosmetic issue when testing an old Atom system (kernel 5.3.y) - there's extra whitespace now present in the CPU model, that wasn't there before.

BEFORE:

AFTER:

/proc/cpuinfo: http://ix.io/21KC

MilhouseVH · 2019-11-15T08:38:45Z

A little more info:

Old kodi.log (before these CPU/SysInfo changes): http://ix.io/21Oj

# grep Atom kodi.log
2019-11-15 08:33:05.757 T:946  NOTICE: Host CPU: Intel(R) Atom(TM) CPU D525 @ 1.80GHz, 4 cores available

New kodi.log (with these latest CPU/SysInfo changes): http://ix.io/21Ok

# grep Atom kodi.log
2019-11-15 08:36:50.422 T:936  NOTICE: Host CPU:          Intel(R) Atom(TM) CPU D525   @ 1.80GHz, 4 cores available

yol

Appreciate you digging into this! Comments added.

xbmc/utils/SysfsUtils.cpp

xbmc/utils/SystemInfo.cpp

xbmc/utils/SysfsUtils.h

xbmc/utils/SysfsUtils.cpp

xbmc/utils/SysfsUtils.h

xbmc/utils/SysfsUtils.cpp

MilhouseVH · 2019-11-25T02:29:36Z

a0b7c71 doesn't apply - it's missing

#include <regex>

that you added in b350f04

lrusak · 2019-11-25T17:22:14Z

a0b7c71 doesn't apply - it's missing
#include <regex>
that you added in b350f04

github says it's fine to merge :/

MilhouseVH · 2019-11-25T17:34:55Z

GNU patch disagrees but OK, will drop it from tonight's test builds.

xbmc/platform/linux/SysfsPath.h

yol · 2019-12-02T21:25:06Z

xbmc/platform/linux/SysfsPath.h

+  template<typename T>
+  bool Get(T& value)
+  {
+    return GetPrivate(value);


Why the indirection?

This is done so I don't have to have duplicated code in the ::Set(std::string) specialization. I mirrored it for ::Get() for uniformity. I can get rid of it though for ::Get() if it's not wanted.

xbmc/platform/linux/SysfsPath.cpp

yol · 2019-12-02T21:26:12Z

xbmc/platform/linux/SysfsPath.cpp

+}
+
+template<>
+bool CSysfsPath::Set(std::string value)


Are you sure this function gets called when you don't declare it in the header?

Yes I am, The tests prove this and so does setting a breakpoint with gdb

(gdb) break CSysfsPath::Set<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >) Breakpoint 1 at 0x19cc03d: CSysfsPath::Set<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >). (2 locations) (gdb) r Starting program: /home/lukas/Documents/git/xbmc/build/kodi-test --gtest_filter=TestSysfsPath\* [Thread debugging using libthread_db enabled] Using host libthread_db library "/lib64/libthread_db.so.1". Missing separate debuginfo for /lib64/libwayland-cursor.so.0 Note: Google Test filter = TestSysfsPath* [==========] Running 2 tests from 1 test case. [----------] Global test environment set-up. [----------] 2 tests from TestSysfsPath [ RUN ] TestSysfsPath.String Breakpoint 1, CSysfsPath::Set<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > (this=0x5a0a090, value='0' <repeats 200 times>...) at /home/lukas/Documents/git/xbmc/xbmc/platform/linux/SysfsPath.cpp:34 warning: Source file is more recent than executable. 34 if (static_cast<int>(value.size()) > PAGESIZE)

Did you try it in the release build?

This function doesn't get called anywhere else then the test.

lrusak · 2020-01-14T01:39:28Z

@yol I dropped the std::string specialisation and tests for std::string I don't think we should be writting strings (characters) to sysfs anyways (plus you need root).

Hopefully we can merge this soon 😸

yol

Tbh I mostly forgot what I complained about months ago so I'm just reviewing based on the current code ;-)

xbmc/platform/linux/SysfsPath.cpp

xbmc/platform/linux/SysfsPath.h

as written in the kernel docs: sysfs allocates a buffer of size (PAGE_SIZE) and passes it to the method. Sysfs will call the method exactly once for each read or write. for more info please consult the kernel documentation: https://www.kernel.org/doc/Documentation/filesystems/sysfs.txt

SysfsUtils: cleanup and improve interface according to the documentation

lrusak added Type: Cleanup non-breaking change which removes non-working or unmaintained functionality Type: Improvement non-breaking change which improves existing functionality Platform: Linux v19 Matrix labels May 8, 2019

lrusak added this to the Matrix 19.0-alpha 1 milestone May 8, 2019

lrusak requested a review from yol May 8, 2019 02:40

lrusak mentioned this pull request May 8, 2019

CCpuInfo split, rework, and cleanup #16098

Merged

6 tasks

yol reviewed May 12, 2019

View reviewed changes

xbmc/utils/AMLUtils.cpp Outdated Show resolved Hide resolved

yol reviewed May 12, 2019

View reviewed changes

xbmc/utils/SysfsUtils.cpp Outdated Show resolved Hide resolved

yol reviewed May 12, 2019

View reviewed changes

xbmc/utils/SysfsUtils.cpp Outdated Show resolved Hide resolved

yol reviewed May 12, 2019

View reviewed changes

thebrid reviewed May 12, 2019

View reviewed changes

xbmc/cores/VideoPlayer/DVDCodecs/Video/AMLCodec.cpp Outdated Show resolved Hide resolved

thebrid reviewed May 12, 2019

View reviewed changes

xbmc/utils/test/TestSysfsUtils.cpp Outdated Show resolved Hide resolved

xbmc/utils/test/TestSysfsUtils.cpp Outdated Show resolved Hide resolved

jenkins4kodi added the Rebase needed PR that does not apply/merge cleanly to current base branch label Jun 6, 2019

lrusak force-pushed the sysfs-cleanup branch from 116a371 to fe567eb Compare June 6, 2019 16:39

jenkins4kodi removed the Rebase needed PR that does not apply/merge cleanly to current base branch label Jun 6, 2019

lrusak force-pushed the sysfs-cleanup branch 2 times, most recently from bc32d42 to b9d748a Compare July 1, 2019 16:10

Rechi reviewed Jul 2, 2019

View reviewed changes

xbmc/utils/SysfsUtils.cpp Outdated Show resolved Hide resolved

xbmc/utils/SysfsUtils.cpp Outdated Show resolved Hide resolved

xbmc/utils/test/TestSysfsUtils.cpp Outdated Show resolved Hide resolved

lrusak force-pushed the sysfs-cleanup branch from b9d748a to 44f3477 Compare October 17, 2019 21:13

Rechi reviewed Oct 17, 2019

View reviewed changes

xbmc/utils/test/TestSysfsUtils.cpp Outdated Show resolved Hide resolved

lrusak force-pushed the sysfs-cleanup branch from 44f3477 to d01d590 Compare October 18, 2019 00:20

lrusak force-pushed the sysfs-cleanup branch 2 times, most recently from 8296589 to 3f6444a Compare October 30, 2019 01:12

lrusak mentioned this pull request Nov 11, 2019

cpuinfo is wrong #16895

Closed

7 tasks

yol reviewed Nov 17, 2019

View reviewed changes

xbmc/utils/SysfsUtils.cpp Show resolved Hide resolved

lrusak force-pushed the sysfs-cleanup branch 2 times, most recently from 0ae1ab2 to a0b7c71 Compare November 25, 2019 00:12

lrusak requested a review from yol November 25, 2019 17:22

yol reviewed Dec 2, 2019

View reviewed changes

lrusak force-pushed the sysfs-cleanup branch from a0b7c71 to 16b19ce Compare December 4, 2019 02:21

remove SysfsUtils.h header from non linux platforms

8d12f60

lrusak force-pushed the sysfs-cleanup branch from 16b19ce to 8089a91 Compare January 14, 2020 01:37

lrusak force-pushed the sysfs-cleanup branch 2 times, most recently from c5af37a to 98ee865 Compare January 14, 2020 16:14

yol reviewed Jan 14, 2020

View reviewed changes

xbmc/platform/linux/SysfsPath.cpp Outdated Show resolved Hide resolved

xbmc/platform/linux/SysfsPath.h Outdated Show resolved Hide resolved

lrusak force-pushed the sysfs-cleanup branch from 98ee865 to 9498d15 Compare January 15, 2020 01:27

lrusak added 3 commits January 14, 2020 19:37

SysfsUtils: move to platform/linux and rename to SysfsPath

af927c1

kodi-test: add test cases for CSysfsPath

7683a1d

lrusak force-pushed the sysfs-cleanup branch from 9498d15 to 7683a1d Compare January 15, 2020 03:38

lrusak merged commit 3a9278b into xbmc:master Jan 15, 2020

Maven85 pushed a commit to Maven85/kodi that referenced this pull request Jan 21, 2020

Merge pull request xbmc#16097 from lrusak/sysfs-cleanup

755b585

SysfsUtils: cleanup and improve interface according to the documentation

MilhouseVH mentioned this pull request Jan 24, 2020

After the Sysutils change we log N/A information #17258

Closed

7 tasks

lrusak mentioned this pull request Jan 29, 2020

[linux] CSysfsPath: fix reading from strings with spaces #17289

Merged

MilhouseVH mentioned this pull request Feb 1, 2020

CPUInfo: remove default "N/A" value from strings #17290

Merged

lrusak deleted the sysfs-cleanup branch February 11, 2022 21:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SysfsUtils: cleanup and improve interface according to the documentation #16097

SysfsUtils: cleanup and improve interface according to the documentation #16097

lrusak commented May 8, 2019

lrusak commented May 11, 2019

yol left a comment

lrusak commented May 16, 2019

Rechi commented May 16, 2019

lrusak commented May 16, 2019 •

edited

Loading

stefansaraev commented May 16, 2019

lrusak commented May 16, 2019

MilhouseVH commented Nov 15, 2019

MilhouseVH commented Nov 15, 2019

yol left a comment

MilhouseVH commented Nov 25, 2019

lrusak commented Nov 25, 2019

MilhouseVH commented Nov 25, 2019

yol Dec 2, 2019

lrusak Dec 4, 2019

yol Dec 2, 2019

lrusak Dec 4, 2019

yol Dec 11, 2019

lrusak Dec 11, 2019

lrusak commented Jan 14, 2020 •

edited

Loading

yol left a comment

SysfsUtils: cleanup and improve interface according to the documentation #16097

SysfsUtils: cleanup and improve interface according to the documentation #16097

Conversation

lrusak commented May 8, 2019

lrusak commented May 11, 2019

yol left a comment

Choose a reason for hiding this comment

lrusak commented May 16, 2019

Rechi commented May 16, 2019

lrusak commented May 16, 2019 • edited Loading

stefansaraev commented May 16, 2019

lrusak commented May 16, 2019

MilhouseVH commented Nov 15, 2019

MilhouseVH commented Nov 15, 2019

yol left a comment

Choose a reason for hiding this comment

MilhouseVH commented Nov 25, 2019

lrusak commented Nov 25, 2019

MilhouseVH commented Nov 25, 2019

yol Dec 2, 2019

Choose a reason for hiding this comment

lrusak Dec 4, 2019

Choose a reason for hiding this comment

yol Dec 2, 2019

Choose a reason for hiding this comment

lrusak Dec 4, 2019

Choose a reason for hiding this comment

yol Dec 11, 2019

Choose a reason for hiding this comment

lrusak Dec 11, 2019

Choose a reason for hiding this comment

lrusak commented Jan 14, 2020 • edited Loading

yol left a comment

Choose a reason for hiding this comment

lrusak commented May 16, 2019 •

edited

Loading

lrusak commented Jan 14, 2020 •

edited

Loading