I think the idea to change to a non allocating variant is good and the patch also looks good.

There are only two things I don't like :-)
- the name of slapi_fetch_attr(), it does not indicate what it does, I would prefer something like slapi_entry_attr_get_reference() and the third param for fetch attr is almodt always NULL, so a new function could clean this up. But it is probably to late.
- we now return a (const char), but in many cases it is just casted to a (char ) - would it be possoble to use const vars ?

I think the idea to change to a non allocating variant is good and the patch also looks good.
There are only two things I don't like :-)
- the name of slapi_fetch_attr(), it does not indicate what it does, I would prefer something like slapi_entry_attr_get_reference() and the third param for fetch attr is almodt always NULL, so a new function could clean this up. But it is probably to late.

Refactoring the name was easy, but refactoring the use of the third parameter is not.

• we now return a (const char), but in many cases it is just casted to a (char ) - would it be possoble to use const vars ?

The problem is that there are many functions that use this returned var, and they are a mix of const vs non-const. Either way there will be casting.

For now I just refactored the name.

Please review...

okay, I figured that making the const stuff clean would require too much work, it is ok then to cast.

I still think it is a bit heavy to replace a function to just get a ref with one requiring an additional unused argument. I would have thought to replace _get_charptr by _get_ref and leave the calls to fetch_attr with a non-null third param in place. The name is not optimal, but it was there before, just make sure the new function is well named.
But it was me complaining about slapi_fetch_attr() - so if you don't want to make changes again, it is ok

okay, I figured that making the const stuff clean would require too much work, it is ok then to cast.
I still think it is a bit heavy to replace a function to just get a ref with one requiring an additional unused argument. I would have thought to replace _get_charptr by _get_ref and leave the calls to fetch_attr with a non-null third param in place. The name is not optimal, but it was there before, just make sure the new function is well named.
But it was me complaining about slapi_fetch_attr() - so if you don't want to make changes again, it is ok

I did change the function name to slapi_entry_attr_get_ref(), but I will restore slapi_fetch_attr() for the calls that use the third parameter, and change the new function to not use that third param.

@lrkispen, okay I made all your requested changes, please review...

I'm back from PTO, so I'm keen to have a look at this too in the next 24 hours :)

Curious why this needs a type cast?

My main concern would be anything that assumes the ref is mutable and then changes the value to feedback to the api, but I'm sure you checked this. Perhaps it's worth keeping const char * on all the types that take these references to be sure? (As week and useless as C's const is ....).

Curious why this needs a type cast?

There is a calling function, further down that expects that type.

And regarding your other comment there are several functions that use this variable. It's a casting nightmare, but casting here actually simplifies things.

Thanks for doing all the work again. Apart from the casting issue, which I think we have to live with, it is a great simplification and improvement

Ack

found an issue: in check_suffix_entryID there is still a free of entryid_str. Didn't matter because the entry was leaking, in my me leak patch I freed the entry and did get a double free.
Hope there are not more frees missed.

found an issue: in check_suffix_entryID there is still a free of entryid_str. Didn't matter because the entry was leaking, in my me leak patch I freed the entry and did get a double free.
Hope there are not more frees missed.

I'll double check all the calls...

@lkrispen - yes there were a few other mistakes. These are fixed in:

https://pagure.io/389-ds-base/pull-request/50524

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

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.