Hello Hyeonggeun,
Sorry I took a bit of time to answer to the next patches, I had to discuss
internally because of the implication a change of API might have there.
On Tue, Jan 20, 2026 at 10:27:41PM +0900, Hyeonggeun Oh wrote:
> Subject: [PATCH 2/3] MINOR: cfgparse-peers: convert to cfg_kw_list
> registration
You should have a commit message here, wrapped to 80 columns.
> ---
>
I presume the extra '---' is because of the --annotate option, but you should
have a commit message which explains what the patch does. The annotations are
good to explain what changed since the latest version of the patch though.
> This patch converts the peers parser from the peers-specific keyword
> registration system (peers_register_keywords/peers_kw_list) to the standard
> cfg_kw_list mechanism used by other cofiguration sections.
>
> This change:
> - Removes extern declarations and uses proper header includes
> - Converts global variables (errmsg, err_code, newpeer, bind_conf) to
> function-local variables for better encapsulation
> - Replaces peers_keywords iteration with cfg_kw_list iteration
> - Preserves rc < 0 (error) and rc > 0 (warning) return value handling for
> backward compatibility with external modules using EXTRA_OBJS
>
> The peers_register_keywords() system was peers-specific and did not integrate
> with the standard configuration keyword infrastructure. Using cfg_kw_list
> ensures consistency with other parsers and enables the keywords to be
> discovered by the -dKall option.
> ---
> src/cfgparse-peers.c | 18 ++++++++++--------
> 1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/src/cfgparse-peers.c b/src/cfgparse-peers.c
> index b7183229e..2e03f8531 100644
> --- a/src/cfgparse-peers.c
> +++ b/src/cfgparse-peers.c
> @@ -571,22 +571,24 @@ int cfg_parse_peers(const char *file, int linenum, char
> **args, int kwm)
> curpeers->disabled = 0;
> }
> else if (*args[0] != 0) {
> - struct peers_kw_list *pkwl;
> + struct cfg_kw_list *kwl;
> int index;
> - int rc = -1;
> + int rc;
>
> - list_for_each_entry(pkwl, &peers_keywords.list, list) {
> - for (index = 0; pkwl->kw[index].kw != NULL; index++) {
> - if (strcmp(pkwl->kw[index].kw, args[0]) == 0) {
> - rc = pkwl->kw[index].parse(args,
> curpeers, file, linenum, &errmsg);
> + list_for_each_entry(kwl, &cfg_keywords.list, list) {
> + for (index = 0; kwl->kw[index].kw != NULL; index++) {
> + if ((kwl->kw[index].section == CFG_PEERS) &&
> + strcmp(kwl->kw[index].kw, args[0]) == 0) {
> + rc = kwl->kw[index].parse(args,
> CFG_PEERS, curpeers->peers_fe, NULL, file, linenum, &errmsg);
> if (rc < 0) {
> ha_alert("parsing [%s:%d] :
> %s\n", file, linenum, errmsg);
> err_code |= ERR_ALERT |
> ERR_FATAL;
> goto out;
> }
> else if (rc > 0) {
> - ha_warning("parsing [%s:%d] :
> %s\n", file, linenum, errmsg);
> - err_code |= ERR_WARN;
> + if (errmsg)
> + ha_warning("parsing
> [%s:%d] : %s\n", file, linenum, errmsg);
> + err_code |= rc;
That's a bit confusing to mix the <> 0 and the |= method here. It will break
the ERR_ALERT and emits a warning instead.
Looking at the other parts of the code we don't use the same method everywhere
unfortunately.
I'm also seeing that the peers_keyword was using a `struct peers *curpeers` in
its (*parse) callback, instead of a proxy, so that would also break the API.
Maybe converting everything to the cfg_kw_list structure was not the right
solution.
The peers_kw_list seems to be already dumped in cfg_dump_registered_keywords(),
so we should just keep that.
The problem seems to be the ERR_* return code which is required...
So we need to break the return code... At least with both methods 0 is the "no
error" code.
After discussing internaly with other developers, we think it might be a bigger
refactoring that what I thought initially. So I don't think it's worse it for
you to go further unfortunately on this subject. We might have to discuss with
people relying on the registering system internally and come up with a unified
solution.
Regards,
--
William Lallemand
