you set the return value, but no longer skip the next part of code. Is this intentionally ?

you set the return value, but no longer skip the next part of code. Is this intentionally ?

Correct, the next part of the code sets the backends as "not busy" otherwise all backends are inaccessible until you restart the server.

I think I am missing something.
We are here because dblayer_restore failed (return_value<>0) before dblayer_start. This will overwrite the dblayer_restore failure with the error from dblayer_start, should not we keep the original error code ?

I think I am missing something.

:-) I know the feeling

We are here because dblayer_restore failed (return_value<>0) before dblayer_start. This will overwrite the dblayer_restore failure with the error from dblayer_start, should not we keep the original error code ?

Actually part of fix prevents dblayer start from overwriting the original error code. What was happening originally was that we had an error, but then dblayer_start would succeed. Then it would return dblayer_start()'s return code of success.

I'm not 100% sure why we even try to run dblayer_start() after a failure, and the intend of overwriting the original error code (from the original source code). So if it fails which error should we return? I guess it makes sense to keep the original error code, and not the new one from dblayer_start() - I'll change that...

@tbordaz, fixed, please review..

I think that if dblayer_recover failed, there is a call to dblayer_start as an attempt to repair something... it is old code and I do not really know if it is still working.
I agree with your comment, it would also make sense to 'goto out' as soon as we detect a failure.

The patch looks good to me. ACK

I'm not 100% sure why we even try to run dblayer_start() after a failure.

Well I think the idea could have been that when the task is run, before running the task the db is closed and afterwards it has to be started, assuming that in he best of all worlds the original state is still available - which I think is not the case.
And why start only on failure I don't know - this code needs to investigated more and cleaned up :-(

I'm not 100% sure why we even try to run dblayer_start() after a failure.

Well I think the idea could have been that when the task is run, before running the task the db is closed and afterwards it has to be started, assuming that in he best of all worlds the original state is still available - which I think is not the case.
And why start only on failure I don't know - this code needs to investigated more and cleaned up :-(

Well I think it was added as a concept/theory, and was not added to address a real problem scenario. But I really don't know for sure. I know that with the old code those failures were basically ignored if dblayer_start() succeeded (like in this case). So I don't think it ever really worked as designed.

For this patch I will leave it as is, but I suspect we could rewrite this function and remove that dblayer_start() code.

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

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.