Re: RADIUS tests and improvements

On Sat, Mar 04, 2023 at 02:23:10PM +1300, Thomas Munro wrote:
> I don't exactly love the PG_TRY()/PG_CATCH() around the
> CHECK_FOR_INTERRUPTS().

> In fact this kind of CFI-with-cleanup problem
> has been haunting me across several projects. For cases that memory
> contexts and resource owners can't help with, I don't currently know
> what else to do here. Better ideas welcome.

Like adding a Open/CloseSocket() in fd.c to control the leaks?

> If I just let that
> socket leak because I know this backend will soon exit, I'd expect a
> knock at the door from the programming police.

Hmm. It seems to me that you'd better have two patches instead of one
here? First, one to introduce the new parameter to control the
timeout, and a second to improve the responsiveness with a
WaitLatch()? If the CFI proves to be an issue, it would be sad to
have to revert the configuration part, which is worth on its own.

> I don't actually know why we have
> src/test/authentication/t/...{password,sasl,peer}..., but then
> src/test/{kerberos,ldap,ssl}/t/001_auth.pl. For this one, I just
> copied the second style, creating src/test/radius/t/001_auth.pl. I
> can't explain why it should be like that, though. If I propose
> another test for PAM, where should it go?

My take would be to keep the number of directories in src/test/ to a
minimum in the long run. Still, this is a case-by-case, as it depends
on if a set of tests needs an expanded set of modules, configuration
files and/or multiple scripts. ssl has its own set of configuration
files with its module, so it makes sense to be independent. ldap has
its LdapServer.pm with a configuration file, again I'm OK with a
separate case. Kerberos has its own README, but IMO it could also be
moved to src/test/authentication/ as it has a simple structure, with
its requirements moved into a different README.

What this patch set does for the RADIUS test is simple enough in
structure that I would also add it in src/test/authentication/. That
means less Make-fu and less Meson-fu.

In 0001, PG_TEST_EXTRA requires radius for the test. This needs an
update of regress.sgml where the values available are listed. I think
that you'd better document that freeradius is required for the test in
one of the README (either create a new one in radius/, or add this
information to the one in authentication, as you feel).
--
Michael