On 24/07/2024 02:37, Michael Paquier wrote:
> On Tue, Jul 23, 2024 at 08:32:29PM +0300, Heikki Linnakangas wrote:
>> All these new tests are a great asset when refactoring this again.
>
> Thanks for doing that. The coverage, especially with v2, is going to
> be really useful.
>
>> Yeah, I'm also not too excited about the additional code in the backend, but
>> I'm also not excited about writing another test C module just for this. I'm
>> inclined to commit this as it is, but we can certainly revisit this later,
>> since it's just test code.
>
> The point would be to rely on the existing injection_points module,
> with a new callback in it. The callbacks could be on a file of their
> own in the module, for clarity.
Hmm, do we want injection_points module to be a dumping ground for
callbacks that are only useful for very specific injection points, in
specific tests? I view it as a more general purpose module, containing
callbacks that are useful for many different tests. Don't get me wrong,
I'm not necessarily against it, and it would be expedient, that's just
not how I see the purpose of injection_points.
> What you have is OK for me anyway, it
> is good to add more options to developers in this area and this gets
> used in core. That's also enough to manipulate the stack in or even
> out of core.
Ok, I kept it that way.
>> Here's a new rebased version with some minor cleanup. Notably, I added docs
>> for the new IS_INJECTION_POINT_ATTACHED() macro.
>
> 0001 looks OK.
>
> + push @events, "backenderror" if $line =~ /error triggered for
> injection point backend-/;
> + push @events, "v2error" if $line =~ /protocol version 2 error
> triggered/;
>
> Perhaps append an "injection_" for these two keywords?
>
> +#include "storage/proc.h"
>
> This inclusion in injection_point.c should not be needed.
>
>> sets the FrontendProtocol global variable, but I think it's more
>> straightforward to have the test code
>
> The last sentence in the commit message of 0002 seems to be
> unfinished.
Fixed.
> Could you run a perltidy on 005_negotiate_encryption.pl? There are a
> bunch of new diffs in it.
Fixed.
Committed, thanks for the review, and thanks Jacob for the testing!
--
Heikki Linnakangas
Neon (https://neon.tech)