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

[Shuah Khan]

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