Why not use the memory functions in ch_malloc.c?

Maybe you can elaborate why this code block is commented out?

Besides those comments this looks good. I am assuming you've run this through ASAN as well? :-)

Why not use the memory functions in ch_malloc.c?

William of the past and the present doesn't like the ch_malloc.c functions. They add indirection and functionality that isn't useful. We have needless extra derefs of pointers, have to cast pointers to get them into free all the time, and it doesn't protect us from much - ie malloc will always succeed and it's only on first write do you actually find out if you get the memory or not because of the way virtual memory works in linux (try it sometime. You can straight up malloc 1TB of memory, and it'll do it, it's only once you write does it remap from the zero page to a real page, so you only OOM once you write more than your systems mem capacy).

So all our checks for malloc succeeding do nothing, we waste cpu cycles on free with extra pointers, and it really doesn't add value. So I wanted to have a replacement for that which basically just is a skinnier wrapper, than boils down to an inlineable != NULL check really.

But also not that it doesn't have malloc, only calloc because calloc is faster for zero'd allocations in most cases, (you remember the past when we malloc and memset everywhere right ;) )

So this is my "first step" in world domin....replacing the ch_malloc functions in the code base :)

Maybe you can elaborate why this code block is commented out?

I had it commented so that in the future if I revive the certmap plugin i could just uncomment it, but honestly, it doesn't need to be there, it won't be hard to recreate later so I'll remove it.

Besides those comments this looks good. I am assuming you've run this through ASAN as well? :-)

Of course! I don't trust myself to write code without ASAN these days, I'm far too silly :)

Why not using slapi_search_internal_pb and keeping search_internal_pb static to plugin_internal_op.c ?

A general question, why do we need so many new v4 definitions that are identical to the current ones ?
slapi_v4_dn, slapi_v4_ch_array, slapi_v4_entry...

The changes are large, does it exist a design of upgrade handlers ?
I fail to find the string "handler" in the patch and to discover how they are called :(

Why not use the memory functions in ch_malloc.c?

William of the past and the present doesn't like the ch_malloc.c functions. They add indirection and functionality that isn't useful.

The one thing I REALLY like about slapi_ch_free() is that you have a pointer to pointer so it can be nullified after being freed. So you know the original pointer is always set to NULL after being freed - prevents a lot of accidental use-after-free errors. I would strongly consider not creating a new free function as I feel slapi_ch_free() is superior :-p But I'm not going to block you on it.

Also I do see a compiler warning as well:

../389-ds-base/ldap/servers/slapd/upgrade.c:22:1: warning: ‘upgrade_entry_exists_or_create’ defined but not used [-Wunused-function]
22 | upgrade_entry_exists_or_create(char upgrade_id, char filter, char dn, char entry) {
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Why not using slapi_search_internal_pb and keeping search_internal_pb static to plugin_internal_op.c ?

Because pblock has a lot of overheads by being a huge structure, with a lot of indirection. This removes the indirection to the minimal surface area needed.

A general question, why do we need so many new v4 definitions that are identical to the current ones ?
slapi_v4_dn, slapi_v4_ch_array, slapi_v4_entry...

Because this was meant to be part of a simplification of the plugin api to remove a lot of legacy junk, and to provide apis and wrappers that could be exposed to rust via ffi (our current api's are a nightmare to get working in rust, which is why the entryuuid plugin has such alarge rust shim layer).

For example, did you know our search api internally demands a plugin id/component id to be use for an access control framework that was never developed?

The changes are large, does it exist a design of upgrade handlers ?
I fail to find the string "handler" in the patch and to discover how they are called :(

I can write a design :)

The one thing I REALLY like about slapi_ch_free() is that you have a pointer to pointer so it can be nullified after being freed. So you know the original pointer is always set to NULL after being freed - prevents a lot of accidental use-after-free errors. I would strongly consider not creating a new free function as I feel slapi_ch_free() is superior :-p But I'm not going to block you on it.

I'm also not so attached to the new ones in this case. William of the past was more invested in it, and while I think it's better to remove and replace them, there is a strong argument to "not change too much" at once. I'll use the ch_free for now then.

Also I do see a compiler warning as well:
../389-ds-base/ldap/servers/slapd/upgrade.c:22:1: warning: ‘upgrade_entry_exists_or_create’ defined but not used [-Wunused-function]
22 | upgrade_entry_exists_or_create(char upgrade_id, char filter, char dn, char entry) {
| ^~

Right, I'll fix this, I'm not sure why this was missed but yeah, I'll fix it.

The changes are large, does it exist a design of upgrade handlers ?
I fail to find the string "handler" in the patch and to discover how they are called :(

I can write a design :)

https://github.com/marcus2376/389wiki/pull/43/files

Turns out I'd already done this in the past as part of the container work. This has just extended it. I think the feature and the concept is pretty simple though, but I can expand more if needed

Ohh and you are looking for "upgrade_server()" in main, it happens after the config dse is started so we can do internal mods.

@firstyear thanks for your answers and the design update. I have a general concern with this patch that it merges two items that should IMHO be done separately: upgrade the server, extending the plugin api.

Regarding the upgrade of the server, I like the idea of server upgrade called early in main. I think it should be made in two phase: functions to upgrade the server, functions to upgrade the plugins. For the plugins, would it be possible to create a new plugin callback level (e.g. SLAPI_PLUGIN_UPGRADE_FN) where plugins can registers their upgrade callbacks and then to call that plugin level in upgrade_server() ?
An advantage is to move each plugin upgrade functions in the plugin source tree (like entryuuid, certmap, PBKDF2...)

Regarding the plugin api evolution I do not see the benefit of moving so much slapi_ function to/under slapi_V4_ functions. I understand that for RUST we do need some enhancements, but why not simply providing those enhancements without changing the name of slapi_ function that do not need to be renamed. For example creating slapi_v4_entry_exists_or_create, slapi_v4_search_internal..
slapi_v4_ would be extension of current plugin slapi_* API.

@firstyear thanks for your answers and the design update. I have a general concern with this patch that it merges two items that should IMHO be done separately: upgrade the server, extending the plugin api.

Yeah, that's a very fair comment :)

Regarding the upgrade of the server, I like the idea of server upgrade called early in main. I think it should be made in two phase: functions to upgrade the server, functions to upgrade the plugins. For the plugins, would it be possible to create a new plugin callback level (e.g. SLAPI_PLUGIN_UPGRADE_FN) where plugins can registers their upgrade callbacks and then to call that plugin level in upgrade_server() ?

I like this idea actually. At the moment though, I don't have a plugin that needs an "on start upgrade" yet though, but I really like this suggestion!

An advantage is to move each plugin upgrade functions in the plugin source tree (like entryuuid, certmap, PBKDF2...)

Correct, but you need the upgrade_server() call to install the plugin config entry first so we can load the plugin to then call it's UPGRADE_FN :)

Regarding the plugin api evolution I do not see the benefit of moving so much slapi_ function to/under slapi_V4_ functions. I understand that for RUST we do need some enhancements, but why not simply providing those enhancements without changing the name of slapi_ function that do not need to be renamed. For example creating slapi_v4_entry_exists_or_create, slapi_v4_search_internal..
slapi_v4_ would be extension of current plugin slapi_* API.

I think I'd be happy to rename these away from slapi_v4 to slapi_*, I think it's hard to see us really rewriting the plugins to a new api given how much momentum we have on the current ones.

So I think I'll re-work this to:

• Use the ch_ memory functions per @mreynolds suggestion
• Remove the v4 in some of the slapi fn names
• Add some comment stubs for where an upgrade_fn hook would be
• fix/check compiler warnings so that @mreynolds doesn't get mad at me :)

The only question though then @tbordaz is that some of the fn's i'm introducing "collide". I have a "simpler" internal add function compared to the current one (IE slapi_v4_add_internal_entry vs slapi_add_internal and slapi_add_entry_internal). So we need to name this in a way that's unique and distinguishes them .... do you have any ideas? I think slapi_add_internal_entry vs slapi_add_entry_internal is really really really confusing :S

@firstyear thanks for your answers and the design update. I have a general concern with this patch that it merges two items that should IMHO be done separately: upgrade the server, extending the plugin api.

Yeah, that's a very fair comment :)

Regarding the upgrade of the server, I like the idea of server upgrade called early in main. I think it should be made in two phase: functions to upgrade the server, functions to upgrade the plugins. For the plugins, would it be possible to create a new plugin callback level (e.g. SLAPI_PLUGIN_UPGRADE_FN) where plugins can registers their upgrade callbacks and then to call that plugin level in upgrade_server() ?

I like this idea actually. At the moment though, I don't have a plugin that needs an "on start upgrade" yet though, but I really like this suggestion!

An advantage is to move each plugin upgrade functions in the plugin source tree (like entryuuid, certmap, PBKDF2...)

Correct, but you need the upgrade_server() call to install the plugin config entry first so we can load the plugin to then call it's UPGRADE_FN :)

Loading of the plugin is done earlier (slapd_bootstrap_config) to upgrade_server, so UPGRADE_FN callbacks should be initialized at that time.

Regarding the plugin api evolution I do not see the benefit of moving so much slapi_ function to/under slapi_V4_ functions. I understand that for RUST we do need some enhancements, but why not simply providing those enhancements without changing the name of slapi_ function that do not need to be renamed. For example creating slapi_v4_entry_exists_or_create, slapi_v4_search_internal..
slapi_v4_ would be extension of current plugin slapi_* API.

I think I'd be happy to rename these away from slapi_v4 to slapi_*, I think it's hard to see us really rewriting the plugins to a new api given how much momentum we have on the current ones.
So I think I'll re-work this to:

Use the ch_ memory functions per @mreynolds suggestion
Remove the v4 in some of the slapi fn names
Add some comment stubs for where an upgrade_fn hook would be
fix/check compiler warnings so that @mreynolds doesn't get mad at me :)

It looks a good summary of the steps. I would just add that I would prefer you separate slapi_ "simpler" extension in a dedicated prerequisite ticket. It is easier to track in the log and focus the review on a specific theme.

The only question though then @tbordaz is that some of the fn's i'm introducing "collide". I have a "simpler" internal add function compared to the current one (IE slapi_v4_add_internal_entry vs slapi_add_internal and slapi_add_entry_internal). So we need to name this in a way that's unique and distinguishes them .... do you have any ideas? I think slapi_add_internal_entry vs slapi_add_entry_internal is really really really confusing :S

I fully agree that naming is important, it will stay forever so better to choose something helpful. I do not really like 'v4' as it does not say what is behind: simplification. I have no strong idea how it should be named, possibly suffixed/prefixed "simple", "easy" , "short", "quick".
I tend to prefer slapi_quick_add_internal_entry vs slapi_add_internal_entry_quick but I think it could be discussed on 389-devel.

Correct, but you need the upgrade_server() call to install the plugin config entry first so we can load the plugin to then call it's UPGRADE_FN :)

Loading of the plugin is done earlier (slapd_bootstrap_config) to upgrade_server, so UPGRADE_FN callbacks should be initialized at that time.

It's loaded in bootstrap but it's not started until plugin_startall which is after upgrade_server, so you'd call the upgrade fn's as part of plugin_startall I think.

Alternately, plugins already have a "start" fn they can use to perform internal upgrades?

I fully agree that naming is important, it will stay forever so better to choose something helpful. I do not really like 'v4' as it does not say what is behind: simplification. I have no strong idea how it should be named, possibly suffixed/prefixed "simple", "easy" , "short", "quick".
I tend to prefer slapi_quick_add_internal_entry vs slapi_add_internal_entry_quick but I think it could be discussed on 389-devel.

Yeah, naming is hard :). I think simple/easy/short/quick all convey the wrong message too somehow ... from all of these, slapi_quick_* is the only one that I really like, but I'm still not sure, I'll have to think about a better name while I'm making the other changes I said I would do :)

It looks a good summary of the steps. I would just add that I would prefer you separate slapi_ "simpler" extension in a dedicated prerequisite ticket. It is easier to track in the log and focus the review on a specific theme.

Sorry forgot this point. I think these API's don't have much use outside of this patch though, so I think keeping them together isn't too bad? Once I make the other changes the scope of the patch should shrink too, so after I do the next commit we can decide then if we still want to split it up?

okay, this update removes the v4 references, uses ch_ memory routines, and some other cleanups.

For now, I've used 140_ as the function name for things we aren't sure of until we have a better name ....

Thanks,

indentation looks funny here :-)

Just to be clear, I didn't have a big problem with the new memory functions, except that slapi_ch_free() was better and should not be "duplicated", but the other ones were fine.

As for the naming of those functions - I hate "140_" :-p I actually liked v4, and I think we should keep it v4 until we all decide on a final name. Besides that I am fine with this patch.

All good, I think if I want to change memory functions, it can happen another time. This keeps the focus of the channge to the migration capability.

I hate 140 too, but I wanted something unique I could easily change when we agreed on a better name. I think 'simple' and 'quick' are not reflective of the intent. I thought of 'ng' since everyone loved ng in their api names around the 2000's as a joke, but you know ...

So I'm happy to put it back to v4 in the places.

Anyway, I'll fix that indentation too. Thanks @mreynolds .

Fixed the indent, I'll change the 140 to v4 later :)

Why not defining also
'typedef struct _slapi_pblock_intop slapi_add_pblock' and
slapi_add_pblock * slapi_140_add_internal_entry(Slapi_Entry entry, const char modifier_name);

I admit it is a bit fuzzy in my mind what are the specificities v3, v4, slapi_140_*

Why not those slapi_search_ be named slapi_140_search

Yeah, the more I think about it the more I don't like v4 either. It should be "simple" or "v2". I guess "v4" was tied to 1.4.x? It's still ambiguous. So "v2", or "simple" makes the most sense to me :-)

Actually, the v4 was because th current plugin api is v3. the 140 is tied to 1.4.x.

Part of me at this point thinks we should name it something like "dmwc" which is an acronym for "doesn't matter, who cares", but at least it's unique ;)

Anyway, I'll probably go with simple or slim maybe.

@mreynolds Change to slim, I'll squash the commits when we are happy to merge :)

I understand the need of knowing the number of returned entries. Does it worth storing it in a new field rather than counting it, on the fly, from pb_plugin_internal_search_op_entries ?

Could you add a comment stating they upgrade_server calls 'upgrade_xxx_yyy' functions, where 'yyy' is a description of the component that needs an upgrade and 'xxx' is the version that requires the component upgrade.

Could you add a comment stating they upgrade_server calls 'upgrade_xxx_yyy' functions, where 'yyy' is a description of the component that needs an upgrade and 'xxx' is the version that requires the component upgrade.

Done!

I understand the need of knowing the number of returned entries. Does it worth storing it in a new field rather than counting it, on the fly, from pb_plugin_internal_search_op_entries ?

This doesn't count on the fly though? It's using the field as set by SLAPI_NENTRIES in plugin_internal_op.c line 646?

What I meant is that pb_plugin_internal_search_num_entries is a new field in _slapi_pblock_intop. It is used to store the number of entries (pb_plugin_internal_search_op_entries) that is also a field in _slapi_pblock_intop.
I was wonder if it worth managing this new field as we can get that number on the fly (iterate over pb_plugin_internal_search_op_entries)

if we need the new field pb_plugin_internal_search_num_entries, you may use slapi_search_pblock_get_num_results() rather than direct access to pb_plugin_internal_search_num_entries

What I meant is that pb_plugin_internal_search_num_entries is a new field in _slapi_pblock_intop. It is used to store the number of entries (pb_plugin_internal_search_op_entries) that is also a field in _slapi_pblock_intop.
I was wonder if it worth managing this new field as we can get that number on the fly (iterate over pb_plugin_internal_search_op_entries)

Ahhhh the new field is there to avoid the iteration over the entries, but also because the _slapi_pblock_intop type doesn't exist inside the slapi_pblock_search subtype. The number of results from a search being part of the operation didn't seem right, but when I did the pblock split the analysis showed that it was always associated to operations. But this is a bit of a departure from that where an internal search isn't associated with an operation, because only fields from pblock_search are needed.

So the alternate option could be to remove the pblock_intop version of the num entries and use the pblock_search field version only instead. But I seem to recall the reason there is a number of entries on operations is we count the number of entries in a mod/add/del which does not have an associated search pb.

So that's why I added the field, because it meant either allocating a huge block of memory for the one extra int or trying to fix a reasonably large piece of techdebt. Counting entries is also O(n) which probably isn't worth it when we already have the count available to us, we just need to store it. So that's what I did here :)

@firstyear thanks for those explanations.

I am sorry but the more I am thinking to that patch the more I think it should be split.
I like and agree with the upgrade part.

Regarding the new interface 'slim', it is a good value. Now I do not see the benefit of introducing slapi_search_pblock (i.e. pb->pb_intop) rather than slapi_pblock. For example pb_intop is far from being simple and we have to extend it to store missing fields that are in other parts of slapi_pblock. 'slim' interface hides useless complexity and offers new services (like test+create) but at the end if we need to return a data structure, pb_intop is not significantly better than slapi_pblock. A risk I can imagine is that we will duplicate fields and later we will not know which one we should use.

In my mind slapi_search_pblock_get_ldapresult/slapi_search_pblock_get_entries... should be part of the slim interface to hide the complexity of slapi_pblock/pb_intop: slapi_slim_search_result/slapi_slim_search_entries...

I actually have a patch in the works (Issue#51156) that needs to do an "upgrade" - adding an attribute to the AES reversible password plugin. So I'll be able to try some of this out as soon as it's pushed.

@firstyear thanks for those explanations.

I am sorry but the more I am thinking to that patch the more I think it should be split.
I like and agree with the upgrade part.

Regarding the new interface 'slim', it is a good value. Now I do not see the benefit of introducing slapi_search_pblock (i.e. pb->pb_intop) rather than slapi_pblock.

I think you are suggesting that I drop the slim interfaces and rewrite the operations in terms of the current apis?

The idea was to create an api that was smaller and more streamlined. We have loads of legacy in our do_op routines that is pointless. There is an ACI framework in there to limit what plugins can do.

So I think the options are:

• leave it as is with a parallel api
• rewrite in terms of existing api and clean out the legacy bits

I think you'd prefer I do the latter. So I'll do that since it seems like the more agreeable way to proceed.

This rewrites the changes in terms of existing apis. Hope this resolves any further issues.

The patch (upgrade part) looks good to me. ACK.

Thanks @firstyear for having removed the API enhancement part from that patch. Just to be sure there is no misunderstanding, I think it worth opening a ticket for the API enhancements (slim) you suggested.

Our current API is sometime too complex for simple things and creating new simpler (slim) interface is a good idea and I would like to see the work you did in that new ticket.
Regarding the 'slim' api my concern was creating a new data structure slapi_search_pblock that is not slapi_pblock light but a portion (significant) of slapi_pblock.
Creating new slapi_slim_search_internal, slapi_slim_add_internal... is a good idea but I think it should return slapi_pblock. We can provide slapi_slim_search_get_result/slapi_slim_search_get_entries... to access the slapi_pblock so that the full 'slim' (simple, V4, 140, or whatever its name) interface will be complete. I hope it clarify my point on the 'slim' interface.

The patch (upgrade part) looks good to me. ACK.

Thanks @firstyear for having removed the API enhancement part from that patch. Just to be sure there is no misunderstanding, I think it worth opening a ticket for the API enhancements (slim) you suggested.

Our current API is sometime too complex for simple things and creating new simpler (slim) interface is a good idea and I would like to see the work you did in that new ticket.
Regarding the 'slim' api my concern was creating a new data structure slapi_search_pblock that is not slapi_pblock light but a portion (significant) of slapi_pblock.
Creating new slapi_slim_search_internal, slapi_slim_add_internal... is a good idea but I think it should return slapi_pblock. We can provide slapi_slim_search_get_result/slapi_slim_search_get_entries... to access the slapi_pblock so that the full 'slim' (simple, V4, 140, or whatever its name) interface will be complete. I hope it clarify my point on the 'slim' interface.

The pblock in it's current form is a hugely inefficient, and confusing, and poorly designed construct. We really should avoid it or work out how to break it down, but that can be a future problem ....

Thanks for having merged the upgrade patch. I think @mreynolds was waiting for it.

Regarding the drop of pblock structure for simple API I think it is a good idea. It can be "radical", defining a slapi_search, slapi_mod, slapi_add... with the minimal required fields (results, rc, entries, controls,...). Those fields being taken from slapi_pblock while the slapi_pblock itself was freed.

@tbordaz I have always been a radical of authentication I think :)

I'll come back to this later, but I think there are a lot of things we can improve in this space. Thanks for being patient with me and always giving your reviews and feedback.

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/4273

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.