Hi, Thanks for the second pass!
▎ The PR_O3_SRV_RENAME approach seems good. Good to hear. ▎ Regarding the stick-table checks: these are not present in del server, ▎ so either del server has a bug, or these checks are unnecessary here. You are right. I removed the stick-table/peer-sync checks to stay consistent with del server. ▎ srv_validate_name(): add server already goes through _srv_parse_init() ▎ which calls invalid_char(), so the call in cli_parse_add_server() is ▎ redundant. And for rename, invalid_char() should be sufficient. Agreed. I removed srv_validate_name() entirely. The rename path now uses invalid_char() for the character-set check (which covers [A-Za-z0-9_:.-], including : for IP:port names) plus an explicit length check against the event data name field and an empty-string check for a cleaner error message. The redundant call in cli_parse_add_server() is also gone. ▎ Formatting: checkpatch will complain but that is not an issue. Alright, good to know, thanks. Then, I am looking forward to your feedback after the internal discussion on the resolver/SRV_F_NON_PURGEABLE question. Best regards, Alexander From: Amaury Denoyelle <[email protected]> Date: Friday, 19. June 2026 at 09:30 To: Stephan, Alexander <[email protected]> Cc: [email protected] <[email protected]> Subject: Re: [PATCH 1/3 2/3 3/3] MINOR: server: add 'option server-rename' and 'set server name' CLI command [You don't often get email from [email protected]. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] Hi, See my comments below, On Thu, Jun 18, 2026 at 02:54:31PM +0000, Stephan, Alexander wrote: > Hi Amaury, > Thanks for the thorough review. I addressed your suggestions in the following > way: > > 1. > I removed the global server-rename keyword. You are right that a defaults > section already covers the use case of enabling renaming across all > backends without repeating the option. The global tunable and its > parser entry have been dropped entirely. Actually, also my ingress controller > implementation didn’t make use of it. Ok good > 1. > SRV_F_NON_PURGEABLE check. The individual use-server loop and srv->trackers > check have been replaced with a single SRV_F_NON_PURGEABLE flag > check, consistent with del server. This also implicitly covers > resolver-based servers as you noted. The error message has been updated to > reflect the broader scope of the rejection. Ok so the solution with SRV_F_NON_PURGEABLE is the simplest. This is annoying though that this prevents renaming in case of servers relying on resolvers. I'll have to talk about that with other developers. Aside from that, I see there is still extra checks in your rename handler beyond SRV_F_NON_PURGEABLE flag, related to stick-table and peer synchronization. I'm not sure this is necessary, however if it is, maybe there is an issue with delete server handler in this case which does not have any of these checks. > 1. > Name-validation refactored into a shared helper. A new srv_validate_name() > function consolidates the empty/length/#-prefix/charset checks. It > is now called by both srv_update_server_name() and > cli_parse_add_server(), so the checks no longer need to be maintained in > multiple places. > Note that the character set intentionally differs from invalid_char() > used by the config parser: the runtime helper allows characters such as > : so that IP:port strings (e.g., 172.17.0.4:8080) remain valid server > names at runtime, which is a primary use case for this feature. I was surprized to see that you have to include a new call to srv_validate_name() in the CLI add server. Thus I digged in the code and in fact server name in this case is validated with invalid_char() via _srv_parse_init(). Thus there is no need to implement a new helper function and invalid_char() should be sufficient for the rename operation as well. > One more remark regarding formatting. The CLI keyword table entry for set > server was realigned to match the surrounding entries. checkpatch warns about > the line length > due to tab expansion, but the alignment is intentional and consistent with > the existing style in that table. I'm not seeing any issue here. Alignment is not based on tab but on space but it's consistent with the code so you can ignore the checkpatch warning in this case. Thanks again. Before deciding in favor or not of a merge, I'll still need some more discussion internally with developers. I'll return to you when it's done. Regards, -- Amaury Denoyelle
0001-MINOR-server-add-option-server-rename-and-set-server.patch
Description: 0001-MINOR-server-add-option-server-rename-and-set-server.patch
0002-REGTESTS-server-add-test-for-set-server-name-CLI-com.patch
Description: 0002-REGTESTS-server-add-test-for-set-server-name-CLI-com.patch
0003-DOC-server-document-option-server-rename-and-set-ser.patch
Description: 0003-DOC-server-document-option-server-rename-and-set-ser.patch
