The provided patch contains three different fixes.

It extends the output of dgben, whIch I think is useful, but there is no relation to the issue of the ticket itself. It is handy for the rewrite of the pytest.

The change of the pytest is also ok, but again it is not really testing the memory leak, just a required change for py3.

The fix for the memory leak is ok, although I would prefer that instances is initialized to NULL. lookup_instance_names_by_suffixes() might not set it. The patch is not affected beacause of handling the return code, but it is safer.

Overall I think everything is ok, but I do not like to merge different things into one patch.
we might want to backport the mem leak fix to another version but not the py3 part.

But we'll want the test to be backported too to show the fix has succeeded? And that will need the python parts to come with it perhaps? I guess I should check with @spichugi and @vashirov about how they plan to test such fixes, so that I can split/merge the fix as appropriate.

Thanks!

In general I agree that patches and test should go together, but :-)

the change in dbgen is not really related to the ticket. It is a change that affects all useage of dbgen, and although I think the change is good. it should not be just a side effect of one patch.

@vashirov Has said "please split the C and lib389" so that's what I'll do. :)

Ohhh, actually, the test won't run without the dbgen change. And the test case is needed to show the issue. So I think it does need to be together ....

okay, improved these two. The free I did caused issues in other modes, so I worked out that we actually need to free the array if we needed to get it.

Second, I found that I didn't commit the new test name, so adding that back.

Regarding 'dbgen.py' and the testcase it is looking fine. Just, when looping on bind attempts in test_referral_during_tot, do we need to open connection. Can't we reuse the same one.

Regarding the fix in slapd_exemode_db2index and slapd_exemode_db2ldif, there are many return in the routine. should we use a goto failure to be sure to free the 'instances' array ?

Hey there. The reason we loop is that the original bug is "during a total init, if we send a bind to the master that is being initialised in referal mode we crash"

So we need a new connection to create the new bind to M2, and we loop to make sure we really get it during the init.

Well, I don't mind the returns - in error cases I'm not sor worried for the little leaks, it's the "success" case, becausu it triggers LSAN for me, which in turns fails the lib389 tests ...

abaou the loop. I think it is ok an required to reconnect, I have two other comments.
The init agreement updates the agreement and returns - and the server will handle the init request, but it can take some time until the consumer is really in referral mode. In a worst case scenario the loop is finished before this.
If in the loop we receive a referral we can break, no need to do it n-times more.

Certainly. That's a great point @lkrispen. The test now exits after one referral. However, the issue with the init not being ready is covered by the loop running multiple times. The issue of the loop finishing to soon is a real valid one, but I don't really have a better solution here? So far this test has been reliable in testing so I think "lets fix that issue when we get to it" is the answer ....

@lkrispen @spichugi can either of your please check this please :)

commit f0b05c054202182c125070ee91b87162d77e0585
commit a3472386ba7419d0da6070a505aebef8be946dcc
To ssh://git@pagure.io/389-ds-base.git
71d7228..72bff95 master -> master

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 issue has been cloned to Github and is available here:
- https://github.com/389ds/389-ds-base/issues/2504

If you want to receive further updates on the issue, please navigate to the github issue
and click on subscribe button.

Thank you for understanding. We apologize for all inconvenience.