linux-kernel - Re: [PATCH v2] cpupower: Make help command available for custom install dir

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CALsPMBOuzzoW4Va9RhgTpTzz3t3kOYN8pKKU5dpiLGbXQOJjxg@mail.gmail.com>
Date: Wed, 26 Jun 2024 20:43:04 +0200
From: Roman Storozhenko <romeusmeister@...il.com>
To: Shuah Khan <skhan@...uxfoundation.org>
Cc: Thomas Renninger <trenn@...e.com>, Shuah Khan <shuah@...nel.org>, 
	Javier Carrasco <javier.carrasco.cruz@...il.com>, linux-pm@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] cpupower: Make help command available for custom
 install dir

On Wed, Jun 26, 2024 at 5:36 PM Shuah Khan <skhan@...uxfoundation.org> wrote:
>
> On 6/26/24 01:29, Roman Storozhenko wrote:
> > On Tue, Jun 25, 2024 at 9:29 PM Shuah Khan <skhan@...uxfoundation.org> wrote:
> >>
> >> On 6/22/24 07:01, Roman Storozhenko wrote:
> >>> 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).
> >>>
> >>> Signed-off-by: Roman Storozhenko <romeusmeister@...il.com>
> >>> ---
> >>> Changes in v2:
> >>> - Fixed spelling errors
> >>> - Simplified man pages search approach by the 'MANPATH' variable usage
> >>> - Link to v1: https://lore.kernel.org/r/20240621-fix-help-issue-v1-1-7906998d46eb@gmail.com
> >>> ---
> >>>    tools/power/cpupower/utils/cpupower.c | 41 ++++++++++++++++++++++++++++++-----
> >>>    1 file changed, 35 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/tools/power/cpupower/utils/cpupower.c b/tools/power/cpupower/utils/cpupower.c
> >>> index 9ec973165af1..1b1b79c572ad 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>
> >>> @@ -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) {
> >>
> >> 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.
> >
>
> 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.

> 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.

>
> thanks,
> -- Shuah
>

-- 
Kind regards,
Roman Storozhenko