should be size_t, and declared inside of the for loops

You don't need the Sun Micro copyright, and you can use 2020 for Red Hat's :-)

This indentation is off ;-)

Indentation is funny here as well.

Indentation...

Most of the logging calls have incorrect indentation on the wrapped line - I'll stop commenting on each one.

@mreynolds thanks for your review. I made the changes you reported.

I think all this debugging boilerplate isn't needed now, topologies does it for you behind the scenes.

Shouldn't this be older than 1.4.4? I think it's a less than check, not less than equal to.

UserAccounts has a "create test user" helper. See src/lib389/lib389/idm/user.py

I don't think anyone runs these directly now, so we probably don't nee the name anymore?

int32_t

Maybe I'm not awake enough yet, where are the rewriters called from in the search path?

@mreynolds @firstyear thanks for your reviews. Patch updated

int32_t here

And here

And this comment thierry:

Maybe I'm not awake enough yet, where are the rewriters called from in the search path?

Thanks by the way, it looks interesting :)

The rewriters are quite hidden in search path. They are called with compute_rewrite_search_filter (opshared.c) and compute_attribute (result.c).

So to be sure, these are only loaded from plugins at startup? These won't be "dynamic" like other plugins (This is a good thing that they are loaded at startup only IMO, dynamic adds a lot of complexity.).

Theorically slapi_compute_add_search_rewriter and slapi_compute_add_evaluator) can be called at anytime during server lifetime. So a new rewriter could be register anytime.

Currently most rewriters are registered at startup (compute_init, entry_computed_attr_init, ldbm_back_start).
An exception is view plugin (views_start) that may register several times the same filter rewriter. The view rewriter looks safe to be registered several times. The only drawback is that it will be called several times.

I agree it is better to register the new rewriters only are startup (less complexity). Another good reason to make the framework in core server than in a plugin ;)

I agree it is better to register the new rewriters only are startup (less complexity). Another good reason to make the framework in core server than in a plugin ;)

Agreed. It's much simpler to accept the server restart, than to try to make these dynamic :) and they'll perform better too as a result!

As a general aside, this PR and your objectCategory PR could be a single PR so we can see the pieces and interactions of both at once maybe? Is there a benefit to splitting them?

Yes I prefer to separate them as it was source of confusion in the previous PR (https://pagure.io/389-ds-base/pull-request/50939) that delivered the new mechanism to register rewriters and a specific rewriter (objectCategory).

we can anticipate several specific rewriters (objectCategory, objectSID, uniquemember,..) that could be part of 389-ds or potentially others projects (freeipa). Rewriters are independent of the new registering mechanism.

I think there are some improvements you could make between https://pagure.io/389-ds-base/pull-request/50988 and this if they were together, as it gives you the full picture?

Anyway, thanks for your patience on this, I appreciate you taking the time on all my comments :)

@firstyear, please have a look at the rebase. Thanks

Use topology_st.standalone.ds_paths.lib_dir instead here. I think you can have:

libslapd = os.path.join( topology_st.standalone.ds_paths.lib_dir, 'dirsrv/libslapd.so')

Which would be much more reliable as a path lookup.

You get the [0] because you are globbing, but if you just use the example I gave above, you don't need the array indexing.

int!!!! int32_t please, like every int should be sized! :)

Better idea than 0 or -1 because that's so oooooo hard to track down in the future, is to use an enum instead. have a look at: https://pagure.io/389-ds-base/pull-request/49579#_30__26

you can even give them int values etc. This way it's much easier to match what int means what, you have to use the enum variants, and the compiler can check when you are returning the wrong type.

This goto isn't needed?

@firstyear the patch is updated according to your comments. Thanks

@firstyear, I would ideally like to merge it this week, do you have any other concern regarding this PR ?

int ........

int ....

int ...

sizeof (entry_string) - 1

= {0}; to zero the stack

int.....

Besides these little comments, I think it's good. I was more thoroguh in the type check today :)

This one was on purpose it is a callback for slapi_filter_apply that is 'typedef int (FILTER_APPLY_FN)(Slapi_Filter f, void *arg);'
I wanted to stick on the definition even if I guess int32_t would be accepted as well

Idem for error_code and rc that stick to
int slapi_filter_apply(struct slapi_filter f, FILTER_APPLY_FN fn, void arg, int *error_code)

similar it is a callback for each returned entry of a search (slapi_search_internal_callback)

typedef int (*plugin_search_entry_callback)(Slapi_Entry *e,  void *callback_data);

It is used to retrieve pblock[SLAPI_PLUGIN_INTOP_RESULT] and sets a 'int', this is why I declared it as an int. Should it preferably be int32_t ?

Thanks @firstyear for your comment. If you agree with the patch but want some changes with int32_t I can do that on the fly and push that patch before the end of the week.

This one was on purpose it is a callback for slapi_filter_apply that is 'typedef int (FILTER_APPLY_FN)(Slapi_Filter f, void *arg);'
I wanted to stick on the definition even if I guess int32_t would be accepted as well

It would be good to comment these :)

It is used to retrieve pblock[SLAPI_PLUGIN_INTOP_RESULT] and sets a 'int', this is why I declared it as an int. Should it preferably be int32_t ?

If it's an array deref then size_t I think is the correct type here ....

Anyway, with those two last comments, ack from me :)

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/4034

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.