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
