Re: Adding facility for injection points (or probe points?) for more advanced tests

On Tue, Jan 9, 2024 at 10:09 AM Michael Paquier <michael@paquier.xyz> wrote:

>
> Okay, I can see your point.  I have reorganized the docs in the
> following order:
> - INJECTION_POINT
> - Attach
> - Detach

Thanks. This looks better. Needs further wordsmithy. But that can wait
till the code has been reviewed.

>
> > I think, exposing the current injection point strings as
> > #defines and encouraging users to use these macros instead of string
> > literals will be a good start.
>
> Nah, I disagree with this one actually.  It is easy to grep for the
> macro INJECTION_POINT to be able to achieve the same research job, and
> this would make the code more inconsistent when callbacks are run
> within extensions which don't care about a #define in a backend
> header.

The macros should be in extension facing header, ofc. But I take back
this suggestion since defining these macros is extra work (every time
a new injection point is declared) and your suggestion to grep
practically works. We can improve things as needed.

>
> > With the current implementation it's possible to "declare" injection
> > point with same name at multiple places. It's useful but is it
> > intended?
>
> Yes.  I would recommend not doing that, but I don't see why there
> would be a point in restricting that, either.

Since an unintentional misspelling might trigger an unintended
injection point. But we will see how much of that happens in practice.

> > +#ifdef USE_INJECTION_POINTS
> > +static bool
> > +file_exists(const char *name)
> >
> > There's similar function in jit.c and dfmgr.c. Can we not reuse that code?
>
> This has been mentioned in a different comment.  Refactored as of
> 0001, but there is something here related to EACCES for the JIT path.
> Seems weird to me that we would not fail if the JIT library cannot be
> accessed when stat() fails.

I agree with this change to jit. Without having search permissions on
every directory in the path, the function can not determine if the
file exists or not. So throwing an error is better than just returning
false which means that
the file does not exist.

>
> > + /* Save the entry */
> > + memcpy(entry_by_name->name, name, sizeof(entry_by_name->name));
> > + entry_by_name->name[INJ_NAME_MAXLEN - 1] = '\0';
> > + memcpy(entry_by_name->library, library, sizeof(entry_by_name->library));
> > + entry_by_name->library[INJ_LIB_MAXLEN - 1] = '\0';
> > + memcpy(entry_by_name->function, function, sizeof(entry_by_name->function));
> > + entry_by_name->function[INJ_FUNC_MAXLEN - 1] = '\0';
> >
> > Most of the code is using strncpy instead of memcpy. Why is this code different?
>
> strncpy() is less used in the backend code.  It comes to a matter of
> taste, IMO.

To me using memcpy implies that the contents of the memory being
copied can be non-character. For a buffer containing a character
string I would prefer strncpy. But I wouldn't argue furher..

>
> Yeah, that's an intended design choice to keep the code simpler and
> faster as there is no need to track the library and function names in
> the local caches or implement something similar to invalidation
> messages for this facility because it would impact performance anyway
> in the call paths.  In short, just don't do that, or use two distinct
> points.

In practice the InjectionPointDetach() and InjectionPointAttach()
calls may not be close by and the user may not be able to figure out
why the injection points are behaviour weird. It may impact
performance but unexpected behaviour should be avoided, IMO.

If nothing else this should be documented.

>
> On Thu, Jan 04, 2024 at 06:22:35PM +0530, Ashutosh Bapat wrote:
> > One more comment on 0001
> > InjectionPointAttach() doesn't test whether the given function exists
> > in the given library. Even if InjectionPointAttach() succeeds,
> > INJECTION_POINT might throw error because the function doesn't exist.
> > This can be seen as an unwanted behaviour. I think
> > InjectionPointAttach() should test whether the function exists and
> > possibly load it as well by adding it to the local cache.
>
> This has the disadvantage of filling the local cache but that may not
> be necessary with an extra load_external_function() in the attach
> path.  I agree to make things safer, but I would do that when
> attempting to run the callback instead.  Perhaps there's an argument
> for the case of somebody replacing a library on-the-fly.  I don't
> really buy it, but people like doing fancy things sometimes.

I am ok with not populating the cache but checking with just
load_external_function(). This is again another ease of use scenario
where a silly mistake by user is caught earlier making user's life
easier. That at least should be the goal of the first cut.

>
> > 0002 comments
> > --- /dev/null
> > +++ b/src/test/modules/test_injection_points/expected/test_injection_points.out
> >
> > When built without injection point support, this test fails. We should
> > add an alternate output file for such a build so that the behaviour
> > with and without injection point support is tested. Or set up things
> > such that the test is not run under make check in that directory. I
> > will prefer the first option.
>
> src/test/modules/Makefile has a safeguard for ./configure, and there's
> one in test_injection_points/meson.build for Meson.  The test is not
> run when the switches are not used, rather than using an alternate
> output file.

With v6 I could run the test when built with enable_injection_point
false. I just ran make check in that folder. Didn't test meson build.

>  There was a different issue when moving the tests to
> src/test/recovery/, though, where we need to make the execution of the
> tests conditional on get_option('injection_points').

>
> > +
> > +SELECT test_injection_points_run('TestInjectionError'); -- error
> > +ERROR: error triggered for injection point TestInjectionError
> > +-- Re-load and run again.
> >
> > What's getting Re-loaded here? \c will create a new connection and
> > thus a new backend. Maybe the comment should say "test in a fresh
> > backend" or something of that sort?
>
> The local cache is reloaded.  Reworded.

We are starting a new backend not "re"loading a cache in an existing
backend per say.

>
> That's the thing here, we don't have an extra condition to check
> after.  The variable sleep is what triggers the stop.  :)
> Perhaps this could be made smarter or with something else, I'm OK to
> revisit that with the polishing for 0003 I'm planning.  We could use a
> separate shared state, for example, but that does not improve the test
> readability, either.

Yeah, I think we have to use another shared state. If the waiting
backend moves ahead without test_injection_point_wake() being called,
that could lead to unexpected and very weird behaviour.

It looks like ConditionVariable just remembers the processes that need
to be woken up during broadcast or signal. But by itself it doesn't
guarantee desired condition when woken up.

>
> Attached is a v7 series.  What do you think?  0004 and 0005 for the
> extra tests still need more discussion and much more polishing, IMO.

Generally I think the 0001 and 0002 are in good shape. However, I
would like them to be more easy to use - like catching simple user
errors that can be easily caught. That saves a lot of frustration
because of unexpected behaviour. I will review 0001 and 0002 from v7
in detail again, but it might take a few days.

--
Best Wishes,
Ashutosh Bapat