| Message ID | 20240621-fix-help-issue-v1-1-7906998d46eb@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Shuah Khan |
| Headers | show |
| Series | cpupower: Make help command available for custom install dir | expand |
On 6/21/24 02:13, Roman Storozhenko wrote: > When the 'cpupower' utility installed in the custom dir, it fails to > render appopriate help info for a particular subcommand: appopriate -> appropriate Spell check the commit message. > $ 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 man pages available for user using the following rule: > Render a man page if it is installed in the custom install dir, otherwise > allow man to search this page by name system-wide as a last resort. > Good find. > Signed-off-by: Roman Storozhenko <romeusmeister@gmail.com> > --- > tools/power/cpupower/utils/cpupower.c | 54 ++++++++++++++++++++++++++++++----- > 1 file changed, 47 insertions(+), 7 deletions(-) > > diff --git a/tools/power/cpupower/utils/cpupower.c b/tools/power/cpupower/utils/cpupower.c > index 9ec973165af1..da4bc6de7494 100644 > --- a/tools/power/cpupower/utils/cpupower.c > +++ b/tools/power/cpupower/utils/cpupower.c > @@ -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> > @@ -21,6 +23,8 @@ > #include "helpers/bitmask.h" > > #define ARRAY_SIZE(x) (sizeof(x)/sizeof(x[0])) > +#define MAN_REL_PATH "/../man/man1/" > +#define MAN_SUFFIX ".1" > > static int cmd_help(int argc, const char **argv); > > @@ -80,14 +84,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,7 +104,40 @@ static int print_man_page(const char *subpage) > strcat(page, subpage); > } > > - execlp("man", "man", page, NULL); > + /* Get current process image name full path */ > + if (readlink("/proc/self/exe", exec_path, PATH_MAX) > 0) { > + > + man_path = malloc(PATH_MAX); > + if (!man_path) { > + free(page); > + return -ENOMEM; > + } > + > + exec_dir = strdup(exec_path); > + if (!exec_dir) { > + free(page); > + free(man_path); > + return -ENOMEM; > + } > + > + *man_path = '\0'; > + strncat(man_path, dirname(exec_dir), strlen(exec_dir)); > + strncat(man_path, MAN_REL_PATH, strlen(MAN_REL_PATH)); > + strncat(man_path, page, strlen(page)); > + strncat(man_path, MAN_SUFFIX, strlen(MAN_SUFFIX)); > + > + free(exec_dir); > + > + /* Check if file exists */ > + if (access(man_path, F_OK) == -1) { > + free(man_path); > + man_path = page; > + } > + } else { > + man_path = page; > + } > + > + execlp("man", "man", man_path, NULL); You can simplify all of this by using getenv() to get the environment variables for the program. Take a look getenv() usages in the kernel sources for reference. > thanks, -- Shuah
On Fri, Jun 21, 2024 at 5:02 PM Shuah Khan <skhan@linuxfoundation.org> wrote: > > On 6/21/24 02:13, Roman Storozhenko wrote: > > When the 'cpupower' utility installed in the custom dir, it fails to > > render appopriate help info for a particular subcommand: > > appopriate -> appropriate > Spell check the commit message. Thanks for highlighting this, will fix. > > > $ 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 man pages available for user using the following rule: > > Render a man page if it is installed in the custom install dir, otherwise > > allow man to search this page by name system-wide as a last resort. > > > > Good find. > > > Signed-off-by: Roman Storozhenko <romeusmeister@gmail.com> > > --- > > tools/power/cpupower/utils/cpupower.c | 54 ++++++++++++++++++++++++++++++----- > > 1 file changed, 47 insertions(+), 7 deletions(-) > > > > diff --git a/tools/power/cpupower/utils/cpupower.c b/tools/power/cpupower/utils/cpupower.c > > index 9ec973165af1..da4bc6de7494 100644 > > --- a/tools/power/cpupower/utils/cpupower.c > > +++ b/tools/power/cpupower/utils/cpupower.c > > @@ -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> > > @@ -21,6 +23,8 @@ > > #include "helpers/bitmask.h" > > > > #define ARRAY_SIZE(x) (sizeof(x)/sizeof(x[0])) > > +#define MAN_REL_PATH "/../man/man1/" > > +#define MAN_SUFFIX ".1" > > > > static int cmd_help(int argc, const char **argv); > > > > @@ -80,14 +84,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,7 +104,40 @@ static int print_man_page(const char *subpage) > > strcat(page, subpage); > > } > > > > - execlp("man", "man", page, NULL); > > + /* Get current process image name full path */ > > + if (readlink("/proc/self/exe", exec_path, PATH_MAX) > 0) { > > + > > + man_path = malloc(PATH_MAX); > > + if (!man_path) { > > + free(page); > > + return -ENOMEM; > > + } > > + > > + exec_dir = strdup(exec_path); > > + if (!exec_dir) { > > + free(page); > > + free(man_path); > > + return -ENOMEM; > > + } > > + > > + *man_path = '\0'; > > + strncat(man_path, dirname(exec_dir), strlen(exec_dir)); > > + strncat(man_path, MAN_REL_PATH, strlen(MAN_REL_PATH)); > > + strncat(man_path, page, strlen(page)); > > + strncat(man_path, MAN_SUFFIX, strlen(MAN_SUFFIX)); > > + > > + free(exec_dir); > > + > > + /* Check if file exists */ > > + if (access(man_path, F_OK) == -1) { > > + free(man_path); > > + man_path = page; > > + } > > + } else { > > + man_path = page; > > + } > > + > > + execlp("man", "man", man_path, NULL); > > You can simplify all of this by using getenv() to get the environment > variables for the program. > > Take a look getenv() usages in the kernel sources for reference. If you mean that I can extract the current working directory and then add relative path to man page to it then yes, I can. But the issue with this approach is that this will work only if I run the binary from its directory, otherwise it fail, because current_working_directory is not the image_path. And there is no environment variable which defines the path to the process's binary. Just in case, I looked to the kernel sources which use getenv() in userspace and also examined the list of the POSIX environment variables: https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap08.html And nothing came to my mind in terms of simplification. So, please suggest me what I could change. > > > > thanks, > -- Shuah >
On 6/21/24 12:18, Roman Storozhenko wrote: > On Fri, Jun 21, 2024 at 5:02 PM Shuah Khan <skhan@linuxfoundation.org> wrote: >> >> On 6/21/24 02:13, Roman Storozhenko wrote: >>> When the 'cpupower' utility installed in the custom dir, it fails to >>> render appopriate help info for a particular subcommand: >> >> appopriate -> appropriate >> Spell check the commit message. > > Thanks for highlighting this, will fix. >> >>> $ 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 man pages available for user using the following rule: >>> Render a man page if it is installed in the custom install dir, otherwise >>> allow man to search this page by name system-wide as a last resort. >>> >> >> Good find. >> >>> Signed-off-by: Roman Storozhenko <romeusmeister@gmail.com> >>> --- >>> tools/power/cpupower/utils/cpupower.c | 54 ++++++++++++++++++++++++++++++----- >>> 1 file changed, 47 insertions(+), 7 deletions(-) >>> >>> diff --git a/tools/power/cpupower/utils/cpupower.c b/tools/power/cpupower/utils/cpupower.c >>> index 9ec973165af1..da4bc6de7494 100644 >>> --- a/tools/power/cpupower/utils/cpupower.c >>> +++ b/tools/power/cpupower/utils/cpupower.c >>> @@ -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> >>> @@ -21,6 +23,8 @@ >>> #include "helpers/bitmask.h" >>> >>> #define ARRAY_SIZE(x) (sizeof(x)/sizeof(x[0])) >>> +#define MAN_REL_PATH "/../man/man1/" >>> +#define MAN_SUFFIX ".1" >>> >>> static int cmd_help(int argc, const char **argv); >>> >>> @@ -80,14 +84,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,7 +104,40 @@ static int print_man_page(const char *subpage) >>> strcat(page, subpage); >>> } >>> >>> - execlp("man", "man", page, NULL); >>> + /* Get current process image name full path */ >>> + if (readlink("/proc/self/exe", exec_path, PATH_MAX) > 0) { >>> + >>> + man_path = malloc(PATH_MAX); >>> + if (!man_path) { >>> + free(page); >>> + return -ENOMEM; >>> + } >>> + >>> + exec_dir = strdup(exec_path); >>> + if (!exec_dir) { >>> + free(page); >>> + free(man_path); >>> + return -ENOMEM; >>> + } >>> + >>> + *man_path = '\0'; >>> + strncat(man_path, dirname(exec_dir), strlen(exec_dir)); >>> + strncat(man_path, MAN_REL_PATH, strlen(MAN_REL_PATH)); >>> + strncat(man_path, page, strlen(page)); >>> + strncat(man_path, MAN_SUFFIX, strlen(MAN_SUFFIX)); >>> + >>> + free(exec_dir); >>> + >>> + /* Check if file exists */ >>> + if (access(man_path, F_OK) == -1) { >>> + free(man_path); >>> + man_path = page; >>> + } >>> + } else { >>> + man_path = page; >>> + } >>> + >>> + execlp("man", "man", man_path, NULL); >> >> You can simplify all of this by using getenv() to get the environment >> variables for the program. >> >> Take a look getenv() usages in the kernel sources for reference. > > If you mean that I can extract the current working directory and then add > relative path to man page to it then yes, I can. But the issue with > this approach > is that this will work only if I run the binary from its directory, > otherwise it fail, > because current_working_directory is not the image_path. And there is no > environment variable which defines the path to the process's binary. > Just in case, I looked to the kernel sources which use getenv() in userspace and > also examined the list of the POSIX environment variables: > https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap08.html > And nothing came to my mind in terms of simplification. > So, please suggest me what I could change. > How about setting MANPATH before running the command? thanks, -- Shuah
diff --git a/tools/power/cpupower/utils/cpupower.c b/tools/power/cpupower/utils/cpupower.c index 9ec973165af1..da4bc6de7494 100644 --- a/tools/power/cpupower/utils/cpupower.c +++ b/tools/power/cpupower/utils/cpupower.c @@ -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> @@ -21,6 +23,8 @@ #include "helpers/bitmask.h" #define ARRAY_SIZE(x) (sizeof(x)/sizeof(x[0])) +#define MAN_REL_PATH "/../man/man1/" +#define MAN_SUFFIX ".1" static int cmd_help(int argc, const char **argv); @@ -80,14 +84,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,7 +104,40 @@ static int print_man_page(const char *subpage) strcat(page, subpage); } - execlp("man", "man", page, NULL); + /* Get current process image name full path */ + if (readlink("/proc/self/exe", exec_path, PATH_MAX) > 0) { + + man_path = malloc(PATH_MAX); + if (!man_path) { + free(page); + return -ENOMEM; + } + + exec_dir = strdup(exec_path); + if (!exec_dir) { + free(page); + free(man_path); + return -ENOMEM; + } + + *man_path = '\0'; + strncat(man_path, dirname(exec_dir), strlen(exec_dir)); + strncat(man_path, MAN_REL_PATH, strlen(MAN_REL_PATH)); + strncat(man_path, page, strlen(page)); + strncat(man_path, MAN_SUFFIX, strlen(MAN_SUFFIX)); + + free(exec_dir); + + /* Check if file exists */ + if (access(man_path, F_OK) == -1) { + free(man_path); + man_path = page; + } + } else { + man_path = page; + } + + execlp("man", "man", man_path, NULL); /* should not be reached */ return -EINVAL;
When the 'cpupower' utility installed in the custom dir, it fails to render appopriate 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 man pages available for user using the following rule: Render a man page if it is installed in the custom install dir, otherwise allow man to search this page by name system-wide as a last resort. Signed-off-by: Roman Storozhenko <romeusmeister@gmail.com> --- tools/power/cpupower/utils/cpupower.c | 54 ++++++++++++++++++++++++++++++----- 1 file changed, 47 insertions(+), 7 deletions(-) --- base-commit: 2102cb0d050d34d50b9642a3a50861787527e922 change-id: 20240619-fix-help-issue-573c40bb6427 Best regards,