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

[Shuah Khan <skhan@linuxfoundation.org>]

On 6/26/24 01:29, Roman Storozhenko wrote:
> On Tue, Jun 25, 2024 at 9:29 PM Shuah Khan <skhan@linuxfoundation.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@gmail.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 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