On Fri, Oct 21, 2022 at 02:10:37PM +0200, Drouvot, Bertrand wrote:
> On 10/21/22 2:58 AM, Michael Paquier wrote:
>> The same approach with keywords first, regex, and exact match could be
>> applied as well for the databases? Perhaps it is just mainly a matter
>> of taste,
>
> Yeah, I think it is.
;)
Still it looks that this makes for less confusion with a minimal
footprint once the new additions are in place.
>> In the same fashion as load_ident(), it seems to me that we
>> need two extra things for this patch:
>> - if !ok (see where we do MemoryContextDelete(hbacxt)), we need to go
>> through new_parsed_lines and free for each line the AuthTokens for the
>> database and user lists.
>> - if ok and new_parsed_lines != NIL, the same cleanup needs to
>> happen.
>
> Right, but I think that should be "parsed_hba_lines != NIL".
For the second case, where we need to free the past contents after a
success, yes.
> Right. To avoid code duplication in the !ok/ok cases, the function
> free_hba_line() has been added in v2: it goes through the list of databases
> and roles tokens and call free_auth_token() for each of them.
Having a small-ish routine for that is fine.
I have spent a couple of hours doing a pass over v2, playing manually
with regex patterns, reloads, the system views and item lists. The
logic was fine, but I have adjusted a few things related to the
comments and the documentation (particularly with the examples,
removing one example and updating one with a regex that has a comma,
needing double quotes). The CI and all my machines were green, and
the test coverage looked sufficient. So, applied. I'll keep an eye
on the buildfarm.
--
Michael
