Hello William. I deeply agree with your opinion. If further discussion on this feature proceeds and a new implementation is needed, I'd be happy to handle the issue if I can be of help.
If that's the case, would it be okay to close this issue and address the problem at https://github.com/haproxy/haproxy/issues/3246? Thanks to the formatting feedback you provided earlier, I should be able to make my next contribution cleanly. If you prefer to keep good-first-issues reserved for new contributors, I'll explore other issues to contribute to. Regards, Hyeonggeun. On Fri, Jan 23, 2026 at 2:49 AM William Lallemand <[email protected]> wrote: > > 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 > >
