Skip to contentSkip to conversation

cpupower: Make help command available for custom install dir

PATCH Superseded Roman Storozhenko 2 participants 5 messages Jun 22, 2024 – Jun 26, 2024 CPU power monitoring

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore
#0 lore raw

PATCH v2 1/1 cpupower: Make help command available for custom install dir

When the 'cpupower' utility installed in the custom dir, it fails to render appropriate help info for a particular subcommand:

$ LD_LIBRARY_PATH=lib64/ bin/cpupower help monitor
with error message like 'No manual entry for cpupower-monitor.1'
The issue is that under the hood it calls 'exec' function with
the following args: 'man cpupower-monitor.1'. In turn, 'man' search
path is defined in '/etc/manpath.config'. Of course it contains only
standard system man paths.
Make subcommands help available for a user by setting up 'MANPATH'
environment variable to the custom installation man pages dir. That
variable value will be prepended to the man pages standard search paths
as described in 'SEARCH PATH' section of MANPATH(5).

---
Changes in v2:
- Fixed spelling errors
- Simplified man pages search approach by the 'MANPATH' variable usage
- Link to v1: https://lore.kernel.org/r/[email protected]

---
 tools/power/cpupower/utils/cpupower.c | 41 ++++++++++++++++++++++++++++++-----
 1 file changed, 35 insertions(+), 6 deletions(-)
tools/power/cpupower/utils/cpupower.c +35 -6
index 9ec973165af1..1b1b79c572ad 100644
@@ -12,6 +12,8 @@
 #include <unistd.h>
 #include <errno.h>
 #include <sched.h>
+#include <libgen.h>
+#include <limits.h>
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <sys/utsname.h>
@@ -80,14 +82,17 @@ static void print_help(void)
 
 static int print_man_page(const char *subpage)
 {
-	int len;
-	char *page;
+	char *page, *man_path, *exec_dir;
+	char exec_path[PATH_MAX];
+	int subpage_len;
 
-	len = 10; /* enough for "cpupower-" */
-	if (subpage != NULL)
-		len += strlen(subpage);
+	if (!subpage)
+		return -EINVAL;
 
-	page = malloc(len);
+	subpage_len = 10; /* enough for "cpupower-" */
+	subpage_len += strlen(subpage);
+
+	page = malloc(subpage_len);
 	if (!page)
 		return -ENOMEM;
 
@@ -97,6 +102,30 @@ static int print_man_page(const char *subpage)
 		strcat(page, subpage);
 	}
 
+	/* Get current process image name full path */
+	if (readlink("/proc/self/exe", exec_path, PATH_MAX) > 0) {
+		exec_dir = strdup(exec_path);
+		if (!exec_dir) {
+			free(page);
+			free(man_path);
+			return -ENOMEM;
+		}
+
+		/* Prepend standard search path for man pages with relative path
+		 * to custom install man directory
+		 */
+		if (asprintf(&man_path, "%s/../man:", dirname(exec_dir)) > 0) {
+			setenv("MANPATH", man_path, 1);
+			free(exec_dir);
+			free(man_path);
+		} else {
+			free(page);
+			free(exec_dir);
+			free(man_path);
+			return -ENOMEM;
+		}
+	}
+
 	execlp("man", "man", page, NULL);
 
 	/* should not be reached */

---
base-commit: 2102cb0d050d34d50b9642a3a50861787527e922
change-id: 20240619-fix-help-issue-573c40bb6427

Best regards,

Signature · 1 line
Roman Storozhenko <[email protected]>
maintainer
#1 lore raw
Quoted 61 lines · Roman Storozhenko

When the 'cpupower' utility installed in the custom dir, it fails to render appropriate help info for a particular subcommand:

$ LD_LIBRARY_PATH=lib64/ bin/cpupower help monitor
with error message like 'No manual entry for cpupower-monitor.1'
The issue is that under the hood it calls 'exec' function with
the following args: 'man cpupower-monitor.1'. In turn, 'man' search
path is defined in '/etc/manpath.config'. Of course it contains only
standard system man paths.
Make subcommands help available for a user by setting up 'MANPATH'
environment variable to the custom installation man pages dir. That
variable value will be prepended to the man pages standard search paths
as described in 'SEARCH PATH' section of MANPATH(5).

---
Changes in v2:
- Fixed spelling errors
- Simplified man pages search approach by the 'MANPATH' variable usage
- Link to v1: https://lore.kernel.org/r/[email protected]

---
  tools/power/cpupower/utils/cpupower.c | 41 ++++++++++++++++++++++++++++++-----
  1 file changed, 35 insertions(+), 6 deletions(-)
tools/power/cpupower/utils/cpupower.c +11 -6
index 9ec973165af1..1b1b79c572ad 100644
@@ -12,6 +12,8 @@
  #include <unistd.h>
  #include <errno.h>
  #include <sched.h>
+#include <libgen.h>
+#include <limits.h>
  #include <sys/types.h>
  #include <sys/stat.h>
  #include <sys/utsname.h>
@@ -80,14 +82,17 @@ static void print_help(void)
 
  static int print_man_page(const char *subpage)
  {
-	int len;
-	char *page;
+	char *page, *man_path, *exec_dir;
+	char exec_path[PATH_MAX];
+	int subpage_len;
 
-	len = 10; /* enough for "cpupower-" */
-	if (subpage != NULL)
-		len += strlen(subpage);
+	if (!subpage)
+		return -EINVAL;
 
-	page = malloc(len);
+	subpage_len = 10; /* enough for "cpupower-" */
+	subpage_len += strlen(subpage);
+
+	page = malloc(subpage_len);
  	if (!page)
  		return -ENOMEM;
 

tools/power/cpupower/utils/cpupower.c static int print_man_page(const char *subpage) · line 106

Quoted 5 lines
tools/power/cpupower/utils/cpupower.c +2 -0
@@ -97,6 +102,30 @@ static int print_man_page(const char *subpage)
  		strcat(page, subpage);
  	}
 
+	/* Get current process image name full path */
+	if (readlink("/proc/self/exe", exec_path, PATH_MAX) > 0) {

Using /proc/self/exe is Linux and platform specific and not a good solution. Did you loom into using argv[0]?

thanks,
-- Shuah

#2 lore raw
Quoted 66 lines · Shuah Khan
Quoted 64 lines · Roman Storozhenko

When the 'cpupower' utility installed in the custom dir, it fails to render appropriate help info for a particular subcommand:

$ LD_LIBRARY_PATH=lib64/ bin/cpupower help monitor
with error message like 'No manual entry for cpupower-monitor.1'
The issue is that under the hood it calls 'exec' function with
the following args: 'man cpupower-monitor.1'. In turn, 'man' search
path is defined in '/etc/manpath.config'. Of course it contains only
standard system man paths.
Make subcommands help available for a user by setting up 'MANPATH'
environment variable to the custom installation man pages dir. That
variable value will be prepended to the man pages standard search paths
as described in 'SEARCH PATH' section of MANPATH(5).

---
Changes in v2:
- Fixed spelling errors
- Simplified man pages search approach by the 'MANPATH' variable usage
- Link to v1: https://lore.kernel.org/r/[email protected]

---
  tools/power/cpupower/utils/cpupower.c | 41 ++++++++++++++++++++++++++++++-----
  1 file changed, 35 insertions(+), 6 deletions(-)
tools/power/cpupower/utils/cpupower.c +11 -6
index 9ec973165af1..1b1b79c572ad 100644
@@ -12,6 +12,8 @@
  #include <unistd.h>
  #include <errno.h>
  #include <sched.h>
+#include <libgen.h>
+#include <limits.h>
  #include <sys/types.h>
  #include <sys/stat.h>
  #include <sys/utsname.h>
@@ -80,14 +82,17 @@ static void print_help(void)
 
  static int print_man_page(const char *subpage)
  {
-     int len;
-     char *page;
+     char *page, *man_path, *exec_dir;
+     char exec_path[PATH_MAX];
+     int subpage_len;
 
-     len = 10; /* enough for "cpupower-" */
-     if (subpage != NULL)
-             len += strlen(subpage);
+     if (!subpage)
+             return -EINVAL;
 
-     page = malloc(len);
+     subpage_len = 10; /* enough for "cpupower-" */
+     subpage_len += strlen(subpage);
+
+     page = malloc(subpage_len);
      if (!page)
              return -ENOMEM;
 
@@ -97,6 +102,30 @@ static int print_man_page(const char *subpage)
              strcat(page, subpage);
      }
 
Quoted 5 lines

tools/power/cpupower/utils/cpupower.c static int print_man_page(const char *subpage) · line 106

Quoted 2 lines
tools/power/cpupower/utils/cpupower.c +2 -0
+     /* Get current process image name full path */
+     if (readlink("/proc/self/exe", exec_path, PATH_MAX) > 0) {

Using /proc/self/exe is Linux and platform specific and not a good solution. Did you loom into using argv[0]?

Yes, it is not the best solution. I would rather prefer to have a portable, POSIX-based one. But after exploring possible options I came to the conclusion that unfortunately such a solution doesn't exist. According to C11 language standard:
"If the value of argc is greater than zero, the string pointed to by argv[0] represents the program name;....".
Notice - program name, not the absolute path to the program. The actual value of argv is under control of the calling environment.
You could look at the nice discussion of the topic for example here:
https://www.reddit.com/r/C_Programming/comments/dgcmhd/exactly_how_reliable_is_argv0_at_being_the/
Besides - this utility is a part of the Linux Kernel source tree and therefore has no requirement of the portability to another OSes.

Quoted 3 lines

thanks,
-- Shuah

Signature · 2 lines
Kind regards,
Roman Storozhenko
maintainer
#3 lore raw
Quoted 73 lines · Roman Storozhenko
Quoted 66 lines · Shuah Khan
Quoted 64 lines · Roman Storozhenko

When the 'cpupower' utility installed in the custom dir, it fails to render appropriate help info for a particular subcommand:

$ LD_LIBRARY_PATH=lib64/ bin/cpupower help monitor
with error message like 'No manual entry for cpupower-monitor.1'
The issue is that under the hood it calls 'exec' function with
the following args: 'man cpupower-monitor.1'. In turn, 'man' search
path is defined in '/etc/manpath.config'. Of course it contains only
standard system man paths.
Make subcommands help available for a user by setting up 'MANPATH'
environment variable to the custom installation man pages dir. That
variable value will be prepended to the man pages standard search paths
as described in 'SEARCH PATH' section of MANPATH(5).

---
Changes in v2:
- Fixed spelling errors
- Simplified man pages search approach by the 'MANPATH' variable usage
- Link to v1: https://lore.kernel.org/r/[email protected]

---
   tools/power/cpupower/utils/cpupower.c | 41 ++++++++++++++++++++++++++++++-----
   1 file changed, 35 insertions(+), 6 deletions(-)
tools/power/cpupower/utils/cpupower.c +11 -6
index 9ec973165af1..1b1b79c572ad 100644
@@ -12,6 +12,8 @@
   #include <unistd.h>
   #include <errno.h>
   #include <sched.h>
+#include <libgen.h>
+#include <limits.h>
   #include <sys/types.h>
   #include <sys/stat.h>
   #include <sys/utsname.h>
@@ -80,14 +82,17 @@ static void print_help(void)
 
   static int print_man_page(const char *subpage)
   {
-     int len;
-     char *page;
+     char *page, *man_path, *exec_dir;
+     char exec_path[PATH_MAX];
+     int subpage_len;
 
-     len = 10; /* enough for "cpupower-" */
-     if (subpage != NULL)
-             len += strlen(subpage);
+     if (!subpage)
+             return -EINVAL;
 
-     page = malloc(len);
+     subpage_len = 10; /* enough for "cpupower-" */
+     subpage_len += strlen(subpage);
+
+     page = malloc(subpage_len);
       if (!page)
               return -ENOMEM;
 
@@ -97,6 +102,30 @@ static int print_man_page(const char *subpage)
               strcat(page, subpage);
       }
 
Quoted 5 lines

tools/power/cpupower/utils/cpupower.c static int print_man_page(const char *subpage) · line 106

Quoted 2 lines
tools/power/cpupower/utils/cpupower.c +2 -0
+     /* Get current process image name full path */
+     if (readlink("/proc/self/exe", exec_path, PATH_MAX) > 0) {

Using /proc/self/exe is Linux and platform specific and not a good solution. Did you loom into using argv[0]?

Quoted 13 lines

Yes, it is not the best solution. I would rather prefer to have a portable, POSIX-based one. But after exploring possible options I came to the conclusion that unfortunately such a solution doesn't exist. According to C11 language standard:
"If the value of argc is greater than zero, the string pointed to by argv[0] represents the program name;....".
Notice - program name, not the absolute path to the program. The actual value of argv is under control of the calling environment.
You could look at the nice discussion of the topic for example here:
https://www.reddit.com/r/C_Programming/comments/dgcmhd/exactly_how_reliable_is_argv0_at_being_the/
Besides - this utility is a part of the Linux Kernel source tree and therefore has no requirement of the portability to another OSes.

Even so, you don't want to move it towards non-portable. I think I asked this before on your previous version of the patch:

What happens when you set the MANPATH before running the command?

thanks,
-- Shuah

#4 lore raw
Quoted 89 lines · Shuah Khan
Quoted 73 lines · Roman Storozhenko
Quoted 66 lines · Shuah Khan
Quoted 64 lines · Roman Storozhenko

When the 'cpupower' utility installed in the custom dir, it fails to render appropriate help info for a particular subcommand:

$ LD_LIBRARY_PATH=lib64/ bin/cpupower help monitor
with error message like 'No manual entry for cpupower-monitor.1'
The issue is that under the hood it calls 'exec' function with
the following args: 'man cpupower-monitor.1'. In turn, 'man' search
path is defined in '/etc/manpath.config'. Of course it contains only
standard system man paths.
Make subcommands help available for a user by setting up 'MANPATH'
environment variable to the custom installation man pages dir. That
variable value will be prepended to the man pages standard search paths
as described in 'SEARCH PATH' section of MANPATH(5).

---
Changes in v2:
- Fixed spelling errors
- Simplified man pages search approach by the 'MANPATH' variable usage
- Link to v1: https://lore.kernel.org/r/[email protected]

---
   tools/power/cpupower/utils/cpupower.c | 41 ++++++++++++++++++++++++++++++-----
   1 file changed, 35 insertions(+), 6 deletions(-)
tools/power/cpupower/utils/cpupower.c +11 -6
index 9ec973165af1..1b1b79c572ad 100644
@@ -12,6 +12,8 @@
   #include <unistd.h>
   #include <errno.h>
   #include <sched.h>
+#include <libgen.h>
+#include <limits.h>
   #include <sys/types.h>
   #include <sys/stat.h>
   #include <sys/utsname.h>
@@ -80,14 +82,17 @@ static void print_help(void)
 
   static int print_man_page(const char *subpage)
   {
-     int len;
-     char *page;
+     char *page, *man_path, *exec_dir;
+     char exec_path[PATH_MAX];
+     int subpage_len;
 
-     len = 10; /* enough for "cpupower-" */
-     if (subpage != NULL)
-             len += strlen(subpage);
+     if (!subpage)
+             return -EINVAL;
 
-     page = malloc(len);
+     subpage_len = 10; /* enough for "cpupower-" */
+     subpage_len += strlen(subpage);
+
+     page = malloc(subpage_len);
       if (!page)
               return -ENOMEM;
 
@@ -97,6 +102,30 @@ static int print_man_page(const char *subpage)
               strcat(page, subpage);
       }
 
Quoted 5 lines

tools/power/cpupower/utils/cpupower.c static int print_man_page(const char *subpage) · line 106

Quoted 2 lines
tools/power/cpupower/utils/cpupower.c +2 -0
+     /* Get current process image name full path */
+     if (readlink("/proc/self/exe", exec_path, PATH_MAX) > 0) {

Using /proc/self/exe is Linux and platform specific and not a good solution. Did you loom into using argv[0]?

Quoted 13 lines

Yes, it is not the best solution. I would rather prefer to have a portable, POSIX-based one. But after exploring possible options I came to the conclusion that unfortunately such a solution doesn't exist. According to C11 language standard:
"If the value of argc is greater than zero, the string pointed to by argv[0] represents the program name;....".
Notice - program name, not the absolute path to the program. The actual value of argv is under control of the calling environment.
You could look at the nice discussion of the topic for example here:
https://www.reddit.com/r/C_Programming/comments/dgcmhd/exactly_how_reliable_is_argv0_at_being_the/
Besides - this utility is a part of the Linux Kernel source tree and therefore has no requirement of the portability to another OSes.

Quoted 1 line

Even so, you don't want to move it towards non-portable.

I tried to find a portable solution one more time but couldn't. If you know how to do this please let me know.

Quoted 4 lines

I think I asked
this before on your previous version of the patch:

What happens when you set the MANPATH before running the command?

Nice catch. Thanks. Will fix this and send v3.

Quoted 4 lines

thanks,
-- Shuah

Signature · 2 lines
Kind regards,
Roman Storozhenko