Hi Christopher,
On Thu, Sep 25, 2025 at 04:04:59PM +0100, Christopher Staite wrote:
> There is currently an srv_queue converter which is capable of taking the
> output of a dynamic name and determining the queue length for a given
> server. In addition there is a sample fetcher for whether a server is
> currently up. This simply combines the two such that srv_is_up can be
> used as a converter too.
Good idea!
> To avoid code duplication, extract the server lookup into a function
> from the existing converter and re-use in both implementations.
>
> Future work might extend this to other sample fetchers for servers, but
> this is probably the most useful for acl routing.
OK. I'd ask you 3 small things:
- first, when modifying an existing feature to prepare a new one, it's
better to split that into two patches: one that splits the existing
function, and a new one that adds the new feature. This way, possible
regressions on the current functions are easier to detect and fix.
Typical regressions that happen when splitting functions are mostly
build-time issues that could produce warnings on older or newer
compilers for example (e.g. unused parameter in the new function).
And this way the new feature is easier to review, and you'll be happy
to see that your patch looks small, self-contained and cleaner.
- mention in the srv_queue doc (then on srv_is_up) a small warning
like this one: "Before using this, please keep in mind that using
this converter on uncontrolled data might allow an external observer
to query the state of any server in the whole configuration, which
might possibly not be acceptable in some environments".
- your patches were space-mangled by your mailer below, I suggest that
you simply attach them so that they remain intact.
Otherwise at first glance this looks good to me.
Thank you!
Willy
