-
Notifications
You must be signed in to change notification settings - Fork 50
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
make the loop:// connector built-in rather than a DSO #5486
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, nice cleanup!
From the coverage reporting at least, it appears that setting testing userid/rolemask for the loop://
connector from the environment variables FLUX_HANDLE_USERID
and FLUX_HANDLE_ROLEMASK
is not covered (likely wasn't before either though). Could that be dropped? If not maybe it should have a couple sanity tests? Meh, just a suggestion though.
src/common/libflux/connector_loop.c
Outdated
{ | ||
loop_ctx_t *c = impl; | ||
|
||
if (option && streq (option, FLUX_OPT_TESTING_USERID)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious if there's a case for this function to be called with a NULL option
, and if not should it just have the following at the top?
if (option == NULL) {
errno = EINVAL;
return -1;
}
Oh I see this just matches the usage in the local connector, but I'm still unsure of the use case for a NULL option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just copied over what was there in the loopback_create()
implementation, but I'll have a look and see if there's a bit more cleanup we can do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fine to me for now if you want to defer cleanup for later. Just something that caught my eye is all..
Looks like codeql is failing (hopefully temporarily) due to an error fetching some ubuntu packages. |
Codecov Report
@@ Coverage Diff @@
## master #5486 +/- ##
==========================================
+ Coverage 83.64% 83.65% +0.01%
==========================================
Files 484 484
Lines 81443 81480 +37
==========================================
+ Hits 68120 68166 +46
+ Misses 13323 13314 -9
|
The environment variables are interpreted inside |
Ah, ok. I pointed it out just in case, if the code is already used and tested then it could be coverage hiccup (or more likely human error on my part) so safe to ignore it! |
Problem: flux_t is typedefed to 'struct flux_handle_struct' but name_type style names are forbidden per RFC 7. Rename to 'struct flux_handle'.
Problem: some code formatting in libflux is not consistent with modern project practices. Make long parameter lists one per line.
Problem: some connector code formatting is not consistent with modern project practices. Fix the following - make long parameter lists one per line - break long && chains to one per line with one indent
Problem: all connector plugins must be built as DSOs which can be inconvenient. The loop:// plugin is rarely used becuase the majority of unit tests that would use it are in libflux and other code in src/common, which is built before src/connectors. Other connectors might be able to take advantage of global state in libflux-core.so to their betterment, were they implemented as builtins. Add an array of builtin connector init functions, currently empty, and search it before we go off looking for DSOs in the file system.
Problem: the loop connector is not useful for testing libflux when built as a connector, because connector dsos are built after src/common. Relocate loop connector to libflux and build it in. The only change to loop.c is renaming the generic init function.
Problem: rpc unit tests test relies on connector level accessors for credentials but loop:// doesn't implement them. Add support for FLUX_OPT_TESTING_USERID and FLUX_OPT_TESTING_ROLEMASK options, as implemented in loopback_create().
Problem: NULL parameters to flux_op_set() or flux_op_get() could cause a segfault. Fail with EINVAL If 'h' or 'option' are NULL. Adjust checks in local and loop connectors to fit this interface. Add unit tests that cover both the generic checks at entry and the specific checks in connector_loop.
Problem: the libtestutil loopback_create() function duplicates loop:// for unit tests that could not rely on loop.so being built, but now that loop:// is built in, it is no longer required. Convert unit tests to use the real loop:// connector and remove the duplicate implementation.
Thanks for the review! I rebased on current master and forced a push and will set MWP. |
This adds the capability for connectors to be built-in instead of requiring them all to be built as DSOs, and converts
loop://
to built-in, which allowed the duplicate implementation in libtestutil to be removed.I started this while looking into replacing
shmem://
with a non-0MQ equivalent as a way to avoid thezmq_ctx_term(3)
hangs when futures are leaked (#5484) . That would need to be built-in, at least for the simple design I had in mind where named channels would be stored in a libflux-core.so global protected by mutexes, but even if that turns out to be a bad idea, this seems like a nice cleanup.