Thanks for the feedback. The patch below unifies the code in a separated function. I have considered using the strchr() approach, but ended up using strtok() because it is simpler and both approaches required a writable buffer anyway. While it's possible to refactor the new function to work with read-only buffers (such as the one directly returned by getenv()), I don't know if it is worth the trouble -- that would only remove one xstrdup()+free() pair.
Maybe it's also worth mentioning that this patch makes it simple (in theory) to add a GNU-like option -M to man, which is just another way to specify a path list. That may be overkill, though. Signed-off-by: Marcel Rodrigues <[email protected]> --- miscutils/man.c | 67 ++++++++++++++++++++++++++------------------------------- 1 file changed, 31 insertions(+), 36 deletions(-) diff --git a/miscutils/man.c b/miscutils/man.c index 5c1fa2c..8959f5e 100644 --- a/miscutils/man.c +++ b/miscutils/man.c @@ -147,12 +147,37 @@ static int show_manpage(const char *pager, char *man_filename, int man, int leve return run_pipe(pager, man_filename, man, level); } +static int add_man_paths(char ***man_path_list, int idx, char *man_paths) +{ + char *path; + if (man_paths == NULL) + return idx; + path = strtok(man_paths, ":"); + while (path) { + char **path_element; + /* Do we already have path? */ + path_element = *man_path_list; + while (*path_element) { + if (strcmp(*path_element, path) == 0) + goto next; + path_element++; + } + *man_path_list = xrealloc_vector(*man_path_list, 4, idx); + (*man_path_list)[idx] = xstrdup(path); + idx++; + next: + path = strtok(NULL, ":"); + } + return idx; +} + int man_main(int argc, char **argv) MAIN_EXTERNALLY_VISIBLE; int man_main(int argc UNUSED_PARAM, char **argv) { parser_t *parser; const char *pager = ENABLE_LESS ? "less" : "more"; char **man_path_list; + char *man_paths; char *sec_list; char *cur_path, *cur_sect; int count_mp, cur_mp; @@ -165,13 +190,12 @@ int man_main(int argc UNUSED_PARAM, char **argv) sec_list = xstrdup("0p:1:1p:2:3:3p:4:5:6:7:8:9"); /* Last valid man_path_list[] is [0x10] */ - count_mp = 0; man_path_list = xzalloc(0x11 * sizeof(man_path_list[0])); - man_path_list[0] = getenv("MANPATH"); - if (!man_path_list[0]) /* default, may be overridden by /etc/man.conf */ + man_paths = xstrdup(getenv("MANPATH")); + count_mp = add_man_paths(&man_path_list, 0, man_paths); + free(man_paths); + if (!count_mp) /* default, may be overridden by /etc/man.conf */ man_path_list[0] = (char*)"/usr/man"; - else - count_mp++; /* Parse man.conf[ig] or man_db.conf */ /* man version 1.6f uses man.config */ @@ -192,37 +216,8 @@ int man_main(int argc UNUSED_PARAM, char **argv) } else if (strcmp("MANDATORY_MANPATH"+10, token[0]) == 0 /* "MANPATH"? */ || strcmp("MANDATORY_MANPATH", token[0]) == 0 - ) { - char *path = token[1]; - while (*path) { - char *next_path; - char **path_element; - - next_path = strchr(path, ':'); - if (next_path) { - *next_path = '\0'; - if (next_path++ == path) /* "::"? */ - goto next; - } - /* Do we already have path? */ - path_element = man_path_list; - while (*path_element) { - if (strcmp(*path_element, path) == 0) - goto skip; - path_element++; - } - man_path_list = xrealloc_vector(man_path_list, 4, count_mp); - man_path_list[count_mp] = xstrdup(path); - count_mp++; - /* man_path_list is NULL terminated */ - /*man_path_list[count_mp] = NULL; - xrealloc_vector did it */ - skip: - if (!next_path) - break; - next: - path = next_path; - } - } + ) + count_mp = add_man_paths(&man_path_list, count_mp, token[1]); if (strcmp("MANSECT", token[0]) == 0) { free(sec_list); sec_list = xstrdup(token[1]); -- 2.1.3 2014-11-26 20:27 GMT-02:00 Denys Vlasenko <[email protected]>: > On Wed, Nov 26, 2014 at 7:39 PM, Marcel Rodrigues <[email protected]> > wrote: > > BusyBox's man is reading both /etc/man.config and $MANPATH to search for > > man-pages, but currently it assumes $MANPATH contains only a single path. > > > > The MANPATH environment variable may hold a colon-separated list of > > directories to be searched by man. The same rationale for PATH > > applies here: packages installed on different prefixes may coexist on the > > same system. > > You reimplemented the code which already exists a few lines down, > in config file parser. > > Can you refactor the code so that you use a single copy of it for both > purposes? >
_______________________________________________ busybox mailing list [email protected] http://lists.busybox.net/mailman/listinfo/busybox
