Hello, On Fri, Jan 09, 2026 at 05:39:11PM +0900, Hyeonggeun Oh wrote: > Subject: Re: [PATCH] MINOR: cfgparse: Refactor "userlist" parser to print it > in -dKall operation > Hello William. > > I'm implementing the "peers" part of issue > https://github.com/haproxy/haproxy/issues/3221. > And I'm curious about how to implement the "peers" parsing logic and > registration of parsing functions. > > Since "peers" cfg parse logic (cfg_parse_peers func) has multiple branches > and various shared variables, It would be better to separate the whole > peers function into another file with the name cfgparse-peers.c. And there > will be new global static variables and registration logics. > > I think this way is good to manage various local variables in > cfg_parse_peers and register keywords clearly. > Please let me know what you think about it.
That seems reasonable in my opinion, would be a good opportunity to split the code in another file ! Regards, > On Thu, Jan 8, 2026 at 2:33 AM William Lallemand <[email protected]> > wrote: > > > Hello, > > > > Thank you, looks good to me. I just changed a little bit your commit > > message and wrap it to 80 columns. I also added a > > fix before your commit on the error message which was mentioning "users" > > instead of "userlist" when a keyword was not > > found ! > > > > Merged in master! > > > > Regards, > > > > On Tue, Jan 06, 2026 at 01:22:12PM +0900, Hyeonggeun Oh wrote: > > > Subject: [PATCH] MINOR: cfgparse: Refactor "userlist" parser to print it > > in -dKall operation > > > Hello haproxy team. > > > This patch covers issue https://github.com/haproxy/haproxy/issues/3221. > > > > > > Before working keyword "peers", which has bigger loggic than "userlist", > > I've fixed "userlist" keyword first to check and make consensus with > > maintainers. > > > > > > The parser for the "userlist" section did not use the standard keyword > > registration mechanism. Instead, it relied on a series of strcmp() > > comparisons to identify keywords such as "group" and "user". > > > > > > This had two main drawbacks: > > > 1. The keywords were not discoverable by the "-dKall" dump option, > > making it difficult for users to see all available keywords for the section. > > > 2. The implementation was inconsistent with the parsers for other > > sections, which have been progressively refactored to use the standard > > cfg_kw_list infrastructure. > > > > > > This patch refactors the userlist parser to align it with the project's > > standard conventions. > > > > > > The parsing logic for the "group" and "user" keywords has been extracted > > from the if/else block in cfg_parse_users() into two new dedicated > > functions: > > > - cfg_parse_users_group() > > > - cfg_parse_users_user() > > > > > > These two keywords are now registered via a dedicated cfg_kw_list, > > making them visible to the rest of the HAPorxy ecosystem, including the > > -dKall dump. > > > > > > I hope that this change makes the userlist parser cleaner, and > > furthermore, provides a clear pattern for refactoring other sections like > > "peers" or the other keywords. > > > --- > > > src/cfgparse.c | 259 ++++++++++++++++++++++++++++--------------------- > > > 1 file changed, 146 insertions(+), 113 deletions(-) > > > > > > diff --git a/src/cfgparse.c b/src/cfgparse.c > > > index 5a7ec7405..08cd3e0d1 100644 > > > --- a/src/cfgparse.c > > > +++ b/src/cfgparse.c > > > @@ -1389,153 +1389,184 @@ cfg_parse_users(const char *file, int linenum, > > char **args, int kwm) > > > newul->next = userlist; > > > userlist = newul; > > > > > > - } else if (strcmp(args[0], "group") == 0) { /* new group */ > > > - int cur_arg; > > > - const char *err; > > > - struct auth_groups *ag; > > > + } else { > > > + const struct cfg_kw_list *kwl; > > > + char *errmsg = NULL; > > > + int index; > > > > > > - if (!*args[1]) { > > > - ha_alert("parsing [%s:%d]: '%s' expects <name> as > > arguments.\n", > > > - file, linenum, args[0]); > > > - err_code |= ERR_ALERT | ERR_FATAL; > > > - goto out; > > > + list_for_each_entry(kwl, &cfg_keywords.list, list) { > > > + for (index = 0; kwl->kw[index].kw; index++) { > > > + if ((kwl->kw[index].section & > > CFG_USERLIST) && > > > + (strcmp(kwl->kw[index].kw, > > args[0]) == 0)) { > > > + err_code |= > > kwl->kw[index].parse(args, CFG_USERLIST, NULL, NULL, file, linenum, > > &errmsg); > > > + if (errmsg) { > > > + ha_alert("parsing > > [%s:%d] : %s\n", file, linenum, errmsg); > > > + ha_free(&errmsg); > > > + } > > > + goto out; > > > + } > > > + } > > > } > > > > > > - err = invalid_char(args[1]); > > > - if (err) { > > > - ha_alert("parsing [%s:%d]: character '%c' is not > > permitted in '%s' name '%s'.\n", > > > - file, linenum, *err, args[0], args[1]); > > > - err_code |= ERR_ALERT | ERR_FATAL; > > > - goto out; > > > - } > > > + ha_alert("parsing [%s:%d]: unknown keyword '%s' in '%s' > > section\n", file, linenum, args[0], "users"); > > > + err_code |= ERR_ALERT | ERR_FATAL; > > > + } > > > > > > - if (!userlist) > > > - goto out; > > > +out: > > > + return err_code; > > > +} > > > > > > - for (ag = userlist->groups; ag; ag = ag->next) > > > - if (strcmp(ag->name, args[1]) == 0) { > > > - ha_warning("parsing [%s:%d]: ignoring > > duplicated group '%s' in userlist '%s'.\n", > > > - file, linenum, args[1], > > userlist->name); > > > - err_code |= ERR_ALERT; > > > - goto out; > > > - } > > > +int cfg_parse_users_group(char **args, int section_type, struct proxy > > *curproxy, const struct proxy *defproxy, const char *file, int linenum, > > char **err) > > > +{ > > > + int cur_arg; > > > + const char *err_str; > > > + struct auth_groups *ag; > > > + int err_code = 0; > > > > > > - ag = calloc(1, sizeof(*ag)); > > > - if (!ag) { > > > - ha_alert("parsing [%s:%d]: out of memory.\n", > > file, linenum); > > > - err_code |= ERR_ALERT | ERR_ABORT; > > > - goto out; > > > - } > > > + if (!*args[1]) { > > > + ha_alert("parsing [%s:%d]: '%s' expects <name> as > > arguments.\n", > > > + file, linenum, args[0]); > > > + err_code |= ERR_ALERT | ERR_FATAL; > > > + goto out; > > > + } > > > > > > - ag->name = strdup(args[1]); > > > - if (!ag->name) { > > > - ha_alert("parsing [%s:%d]: out of memory.\n", > > file, linenum); > > > - err_code |= ERR_ALERT | ERR_ABORT; > > > - free(ag); > > > + err_str = invalid_char(args[1]); > > > + if (err_str) { > > > + ha_alert("parsing [%s:%d]: character '%c' is not permitted > > in '%s' name '%s'.\n", > > > + file, linenum, *err_str, args[0], args[1]); > > > + err_code |= ERR_ALERT | ERR_FATAL; > > > + goto out; > > > + } > > > + > > > + if (!userlist) > > > + goto out; > > > + > > > + for (ag = userlist->groups; ag; ag = ag->next) > > > + if (strcmp(ag->name, args[1]) == 0) { > > > + ha_warning("parsing [%s:%d]: ignoring duplicated > > group '%s' in userlist '%s'.\n", > > > + file, linenum, args[1], > > userlist->name); > > > + err_code |= ERR_ALERT; > > > goto out; > > > } > > > > > > - cur_arg = 2; > > > + ag = calloc(1, sizeof(*ag)); > > > + if (!ag) { > > > + ha_alert("parsing [%s:%d]: out of memory.\n", file, > > linenum); > > > + err_code |= ERR_ALERT | ERR_ABORT; > > > + goto out; > > > + } > > > > > > - while (*args[cur_arg]) { > > > - if (strcmp(args[cur_arg], "users") == 0) { > > > - if (ag->groupusers) { > > > - ha_alert("parsing [%s:%d]: 'users' > > option already defined in '%s' name '%s'.\n", > > > - file, linenum, args[0], > > args[1]); > > > - err_code |= ERR_ALERT | ERR_FATAL; > > > - free(ag->groupusers); > > > - free(ag->name); > > > - free(ag); > > > - goto out; > > > - } > > > - ag->groupusers = strdup(args[cur_arg + 1]); > > > - cur_arg += 2; > > > - continue; > > > - } else { > > > - ha_alert("parsing [%s:%d]: '%s' only > > supports 'users' option.\n", > > > - file, linenum, args[0]); > > > + ag->name = strdup(args[1]); > > > + if (!ag->name) { > > > + ha_alert("parsing [%s:%d]: out of memory.\n", file, > > linenum); > > > + err_code |= ERR_ALERT | ERR_ABORT; > > > + free(ag); > > > + goto out; > > > + } > > > + > > > + cur_arg = 2; > > > + > > > + while (*args[cur_arg]) { > > > + if (strcmp(args[cur_arg], "users") == 0) { > > > + if (ag->groupusers) { > > > + ha_alert("parsing [%s:%d]: 'users' option > > already defined in '%s' name '%s'.\n", > > > + file, linenum, args[0], > > args[1]); > > > err_code |= ERR_ALERT | ERR_FATAL; > > > free(ag->groupusers); > > > free(ag->name); > > > free(ag); > > > goto out; > > > } > > > + ag->groupusers = strdup(args[cur_arg + 1]); > > > + cur_arg += 2; > > > + continue; > > > + } else { > > > + ha_alert("parsing [%s:%d]: '%s' only supports > > 'users' option.\n", > > > + file, linenum, args[0]); > > > + err_code |= ERR_ALERT | ERR_FATAL; > > > + free(ag->groupusers); > > > + free(ag->name); > > > + free(ag); > > > + goto out; > > > } > > > + } > > > > > > - ag->next = userlist->groups; > > > - userlist->groups = ag; > > > + ag->next = userlist->groups; > > > + userlist->groups = ag; > > > > > > - } else if (strcmp(args[0], "user") == 0) { /* new > > user */ > > > - struct auth_users *newuser; > > > - int cur_arg; > > > +out: > > > + return err_code; > > > +} > > > > > > - if (!*args[1]) { > > > - ha_alert("parsing [%s:%d]: '%s' expects <name> as > > arguments.\n", > > > - file, linenum, args[0]); > > > - err_code |= ERR_ALERT | ERR_FATAL; > > > - goto out; > > > - } > > > - if (!userlist) > > > - goto out; > > > +int cfg_parse_users_user(char **args, int section_type, struct proxy > > *curproxy, const struct proxy *defproxy, const char *file, int linenum, > > char **err) > > > +{ > > > + struct auth_users *newuser; > > > + int cur_arg; > > > + int err_code = 0; > > > > > > - for (newuser = userlist->users; newuser; newuser = > > newuser->next) > > > - if (strcmp(newuser->user, args[1]) == 0) { > > > - ha_warning("parsing [%s:%d]: ignoring > > duplicated user '%s' in userlist '%s'.\n", > > > - file, linenum, args[1], > > userlist->name); > > > - err_code |= ERR_ALERT; > > > - goto out; > > > - } > > > + if (!*args[1]) { > > > + ha_alert("parsing [%s:%d]: '%s' expects <name> as > > arguments.\n", > > > + file, linenum, args[0]); > > > + err_code |= ERR_ALERT | ERR_FATAL; > > > + goto out; > > > + } > > > + if (!userlist) > > > + goto out; > > > > > > - newuser = calloc(1, sizeof(*newuser)); > > > - if (!newuser) { > > > - ha_alert("parsing [%s:%d]: out of memory.\n", > > file, linenum); > > > - err_code |= ERR_ALERT | ERR_ABORT; > > > + for (newuser = userlist->users; newuser; newuser = newuser->next) > > > + if (strcmp(newuser->user, args[1]) == 0) { > > > + ha_warning("parsing [%s:%d]: ignoring duplicated > > user '%s' in userlist '%s'.\n", > > > + file, linenum, args[1], userlist->name); > > > + err_code |= ERR_ALERT; > > > goto out; > > > } > > > > > > - newuser->user = strdup(args[1]); > > > + newuser = calloc(1, sizeof(*newuser)); > > > + if (!newuser) { > > > + ha_alert("parsing [%s:%d]: out of memory.\n", file, > > linenum); > > > + err_code |= ERR_ALERT | ERR_ABORT; > > > + goto out; > > > + } > > > + > > > + newuser->user = strdup(args[1]); > > > > > > - newuser->next = userlist->users; > > > - userlist->users = newuser; > > > + newuser->next = userlist->users; > > > + userlist->users = newuser; > > > > > > - cur_arg = 2; > > > + cur_arg = 2; > > > > > > - while (*args[cur_arg]) { > > > - if (strcmp(args[cur_arg], "password") == 0) { > > > + while (*args[cur_arg]) { > > > + if (strcmp(args[cur_arg], "password") == 0) { > > > #ifdef USE_LIBCRYPT > > > - if (!crypt("", args[cur_arg + 1])) { > > > - ha_alert("parsing [%s:%d]: the > > encrypted password used for user '%s' is not supported by crypt(3).\n", > > > - file, linenum, > > newuser->user); > > > - err_code |= ERR_ALERT | ERR_FATAL; > > > - goto out; > > > - } > > > -#else > > > - ha_warning("parsing [%s:%d]: no crypt(3) > > support compiled, encrypted passwords will not work.\n", > > > - file, linenum); > > > - err_code |= ERR_ALERT; > > > -#endif > > > - newuser->pass = strdup(args[cur_arg + 1]); > > > - cur_arg += 2; > > > - continue; > > > - } else if (strcmp(args[cur_arg], > > "insecure-password") == 0) { > > > - newuser->pass = strdup(args[cur_arg + 1]); > > > - newuser->flags |= AU_O_INSECURE; > > > - cur_arg += 2; > > > - continue; > > > - } else if (strcmp(args[cur_arg], "groups") == 0) { > > > - newuser->u.groups_names = > > strdup(args[cur_arg + 1]); > > > - cur_arg += 2; > > > - continue; > > > - } else { > > > - ha_alert("parsing [%s:%d]: '%s' only > > supports 'password', 'insecure-password' and 'groups' options.\n", > > > - file, linenum, args[0]); > > > + if (!crypt("", args[cur_arg + 1])) { > > > + ha_alert("parsing [%s:%d]: the encrypted > > password used for user '%s' is not supported by crypt(3).\n", > > > + file, linenum, newuser->user); > > > err_code |= ERR_ALERT | ERR_FATAL; > > > goto out; > > > } > > > +#else > > > + ha_warning("parsing [%s:%d]: no crypt(3) support > > compiled, encrypted passwords will not work.\n", > > > + file, linenum); > > > + err_code |= ERR_ALERT; > > > +#endif > > > + newuser->pass = strdup(args[cur_arg + 1]); > > > + cur_arg += 2; > > > + continue; > > > + } else if (strcmp(args[cur_arg], "insecure-password") == > > 0) { > > > + newuser->pass = strdup(args[cur_arg + 1]); > > > + newuser->flags |= AU_O_INSECURE; > > > + cur_arg += 2; > > > + continue; > > > + } else if (strcmp(args[cur_arg], "groups") == 0) { > > > + newuser->u.groups_names = strdup(args[cur_arg + > > 1]); > > > + cur_arg += 2; > > > + continue; > > > + } else { > > > + ha_alert("parsing [%s:%d]: '%s' only supports > > 'password', 'insecure-password' and 'groups' options.\n", > > > + file, linenum, args[0]); > > > + err_code |= ERR_ALERT | ERR_FATAL; > > > + goto out; > > > } > > > - } else { > > > - ha_alert("parsing [%s:%d]: unknown keyword '%s' in '%s' > > section\n", file, linenum, args[0], "users"); > > > - err_code |= ERR_ALERT | ERR_FATAL; > > > } > > > > > > out: > > > @@ -4921,6 +4952,8 @@ REGISTER_CONFIG_SECTION("traces", > > cfg_parse_traces, NULL); > > > > > > static struct cfg_kw_list cfg_kws = {{ },{ > > > { CFG_GLOBAL, "default-path", cfg_parse_global_def_path }, > > > + { CFG_USERLIST, "group", cfg_parse_users_group }, > > > + { CFG_USERLIST, "user", cfg_parse_users_user }, > > > { /* END */ } > > > }}; > > > > > > -- > > > 2.48.1 > > > > > > > > > > > > > -- > > William Lallemand > > > >
