On Thu, Jan 11, 2024 at 9:42 AM Michael Paquier <mich...@paquier.xyz> wrote: > > >> 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. > > In all the infrastructures I've looked at, folks did not really care > about having an invalidation for the callbacks loaded. Still I'm OK > to add something in the documentation about that, say among the lines > of an extra sentence like: > "The callbacks loaded by a process are cached within each process. > There is no invalidation facility for the callbacks attached to > injection points, hence updating a callback for an injection point > requires a restart of the process to release its cache and the > previous callbacks attached to it."
It doesn't behave exactly like that either. If the INJECTION_POINT is run after detach (but before Attach), the local cache will be updated. A subsequent attach and INJECTION_POINT call would fetch the new callback. > > > 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. > > I don't really aim for complicated here, just useful. It isn't complicated. Such simple error check improve user's confidence on the feature and better be part of the 1st cut. > > > 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. > > The CI has been failing because 041_invalid_checkpoint_after_promote > was loading Time::HiRes::nanosleep and Windows does not support it. Some miscommunication here. The SQL test under injection_point module can be run in a build without injection_point and it fails. I think it's better to have an alternate output for the same or prohibit the test running itself. -- Best Wishes, Ashutosh Bapat