Both of these seem to be triggered in bak2db

ack

commit 775a58cd44e08a4a558e0a4c92a1ce25edd05181
To ssh://git@pagure.io/389-ds-base.git
18b8ceb..85807be master -> master

You moved this to "new" are there still issues here?

Yeah, the strdup in ldbm. I haven't solved that yet.

The dblayer_post_close part of the patch is doing the opposite of https://fedorahosted.org/389/ticket/47654. Where free of the private dbhome/home was done only during shutdown.
Can it be freed during db2bak/bak2db ?

The compute init part looks good to me.

The dblayer_post_close part of the patch is doing the opposite of https://fedorahosted.org/389/ticket/47654. Where free of the private dbhome/home was done only during shutdown.
Can it be freed during db2bak/bak2db ?

That could be a historical thing: certainly ASAN/LSAN is much much better than valgrind in my experience. With this patch we prevent the leak, and in tests we also have no use-after-free or other odd behaviours / crashes. Would this be better if I ran this assertion with the full test suite rather than just basic + some other suites? Just due to the complexity of the LDBM code, it's often hard to make complete assertions like this without runtime testing.

The compute init part looks good to me.

Thanks! It was weird to track, but I realised it was a double init :)

I agree it can be a historical thing and it is difficult to evaluate. Now I feel strange that this patch is doing the opposite of https://fedorahosted.org/389/ticket/47654 that is quite recent. @mreynolds do you mind to have a look and check if those frees restricted to shutdown ring any bell ?

For the rest of patch, I agree that starting computing attribute make no sense in db2archive/archive2db. So you have my ack for this part, just waiting for @mreynolds feedback.

William can you test this fix under ASAN and run database tasks like db2bak, db2bak.pl, db2ldif.pl, db2index.pl, etc. The fix you are undoing with your patch addressed issues related to these tasks.

Sure thing, I'll run those tasks to be sure. I think it was those tasks that caused the leak in the first place during my tests, but it's good to be sure. I really appreciate the feedback from both of you on this,

Basic test does this:

    # Test online backup
    try:
        topology_st.standalone.tasks.db2bak(backup_dir=backup_dir,
                                            args={TASK_WAIT: True})
    except ValueError:
        log.fatal('test_basic_backup: Online backup failed')
        assert False
    # Test online restore
    try:
        topology_st.standalone.tasks.bak2db(backup_dir=backup_dir,
                                            args={TASK_WAIT: True})
    except ValueError:
        log.fatal('test_basic_backup: Online restore failed')
        assert False
    # Test offline backup
    if not topology_st.standalone.db2bak(backup_dir):
        log.fatal('test_basic_backup: Offline backup failed')
        assert False
    # Test offline restore
    if not topology_st.standalone.bak2db(backup_dir):
        log.fatal('test_basic_backup: Offline backup failed')
        assert False

ticket48270_test.py does an online reindex:

All of these tests pass with this patch :) If anything, there is one more leak to resolve in the offline version of bak2db.

=================================================================
==1798==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 80 byte(s) in 1 object(s) allocated from:
    #0 0x7f9f9e6cfc70 in realloc (/lib64/libasan.so.4+0xdec70)
    #1 0x7f9f9bc7202e  (/lib64/libnspr4.so+0x1102e)
    #2 0x60400007328f  (<unknown module>)
Objects leaked above:
0x60700006c350 (80 bytes)
Direct leak of 49 byte(s) in 1 object(s) allocated from:
    #0 0x7f9f9e668268 in __interceptor_strdup (/lib64/libasan.so.4+0x77268)
    #1 0x7f9f9df74d38 in slapi_ch_strdup (/opt/dirsrv/lib/dirsrv/libslapd.so.0+0x122d38)
    #2 0x7f9f906d59b5 in dblayer_restore (/opt/dirsrv/lib/dirsrv/plugins/libback-ldbm.so+0x8d9b5)
    #3 0x7f9f906e4c5d in ldbm_back_archive2ldbm (/opt/dirsrv/lib/dirsrv/plugins/libback-ldbm.so+0x9cc5d)
    #4 0x41900c in slapd_exemode_archive2db.lto_priv.83 (/opt/dirsrv/sbin/ns-slapd+0x41900c)
    #5 0x473762 in main (/opt/dirsrv/sbin/ns-slapd+0x473762)
    #6 0x7f9f9b4485fd in __libc_start_main (/lib64/libc.so.6+0x205fd)
Objects leaked above:
0x606000130dc0 (49 bytes)
SUMMARY: AddressSanitizer: 129 byte(s) leaked in 2 allocation(s).

Okay, so it's more complex than I thought. It looks like we create the value:

    at /home/william/development/389ds/ds/ldap/servers/slapd/back-ldbm/dblayer.c:6633
6633        priv->dblayer_home_directory = slapi_ch_strdup(li->li_directory);

Watching the pointer, we can see it's changed POST restore:

Hardware watchpoint 5: *0x6110001d5148
Old value = 1248704
New value = 1252640
dblayer_start (li=0x612000040240, dbmode=4112) at /home/william/development/389ds/ds/ldap/servers/slapd/back-ldbm/dblayer.c:1350
1350        priv->dblayer_cachesize = li->li_dbcachesize;
(gdb) bt
#0  dblayer_start (li=0x612000040240, dbmode=4112) at /home/william/development/389ds/ds/ldap/servers/slapd/back-ldbm/dblayer.c:1350
#1  0x00007fffe88d393f in dblayer_restore (li=0x612000040240, src_dir=0x606000119120 "/opt/dirsrv/var/lib/dirsrv/slapd-standalone_1/bak", task=0x0, bename=0x0)
    at /home/william/development/389ds/ds/ldap/servers/slapd/back-ldbm/dblayer.c:6982
#2  0x00007fffe889f2a4 in ldbm_back_archive2ldbm (pb=0x6080001a35a0) at /home/william/development/389ds/ds/ldap/servers/slapd/back-ldbm/archive.c:165
#3  0x0000000000452da2 in slapd_exemode_archive2db () at /home/william/development/389ds/ds/ldap/servers/slapd/main.c:2538
#4  0x00000000004488ba in main (argc=6, argv=0x7fffffffe478) at /home/william/development/389ds/ds/ldap/servers/slapd/main.c:889

So the problematic line is:

#1  0x00007fffe88d393f in dblayer_restore (li=0x612000040240, src_dir=0x606000119120 "/opt/dirsrv/var/lib/dirsrv/slapd-standalone_1/bak", task=0x0, bename=0x0)
    at /home/william/development/389ds/ds/ldap/servers/slapd/back-ldbm/dblayer.c:6982
6977        else /* on-line mode */
6978        {
6979            allinstance_set_not_busy(li);
6980        }
6981    
6982        tmp_rval = dblayer_start(li, dbmode);
6983        if (0 != tmp_rval) {
6984            slapi_log_err(SLAPI_LOG_ERR,
6985                           "dblayer_restore", "Failed to init database\n");
6986            if (task) {

Where we trigger the dblayer_start, and that blows away the priv fields leading to the leak. I'm not sure how to resolve this though, because I would assume we need the dblayer_start here. Perhaps we just need the dblayer_start code to check if priv->dblayer_home_directory != NULL and free it?

William, again you have a bunch of indentation issues. I will review the patch after you fix it. Tip of the day, before you do a git commit , do a git diff and review your own work. Takes 2 minutes. There is no reason for this, and all it does is delay development.

The reverse of this is also true: By insisting we fix this every patch we expend time on a problem that we are soon to resolve by reformatting the code base. It's a waste of time to fix every patch to match, when we are about to "undo" the work anyway.

I would rather start to re-format the offending files than to continue to try and fix individual functions and such.

I recently did this to the pblock code, so maybe it's time for us to start to apply clang format to some other files. Yes, it's a big piece of work / change, but it will long term prevent this issue.

What do you think?

The reverse of this is also true: By insisting we fix this every patch we expend time on a problem that we are soon to resolve by reformatting the code base. It's a waste of time to fix every patch to match, when we are about to "undo" the work anyway.

I'm sorry, but this is a chronic problem with you. I'm trying to help you - I'm not trying to be a jerk (I promise). And if it has to slow down development for now then so be it, because it will save time in the long run.

I would rather start to re-format the offending files than to continue to try and fix individual functions and such.

This last patch only had a few lines of wrong indentation - you shouldn't need to do a separate patch to fix it. Now I get it if you are doing a major code change, but you do this in all of your patches, small and large.

I recently did this to the pblock code, so maybe it's time for us to start to apply clang format to some other files. Yes, it's a big piece of work / change, but it will long term prevent this issue.
What do you think?

I think you should take your time and put out quality fixes, instead of ignoring our coding guidelines and rushing out sloppy work. I am completely baffled why you still think this is okay. It's disrespectful to the team and the project IMO. What bothers me more is that you are not reviewing your own patches (or checking for compiler warnings, etc). 389 is not some amateur side project - it is an enterprise class Directory Server, and you should treat it as such.

I just want the quality of your code to match the quality of your designs. I really am trying to help you here (don't say I didn't try). So please take this all with a grain of salt.

I'm sorry, but this is a chronic problem with you. I'm trying to help you - I'm not trying to be a jerk (I promise). And if it has to slow down development for now then so be it, because it will save time in the long run.

I know you aren't trying to be a jerk - I never assumed that :)

This is the "point" I'm making. How long do we continue to "slow" development by trying to maintain (I think 3) different whitespace guidelines in our code? I know where you are coming from, that it's easy to just do the little bit of work now to change my editor and fix the patch, but like you say. This is a chronic problem for me, and it keeps happening. It's probably going to affect others too. So at what point do we say "enough is enough", and fix our code base to have one whitespacing?

This last patch only had a few lines of wrong indentation - you shouldn't need to do a separate patch to fix it. Now I get it if you are doing a major code change, but you do this in all of your patches, small and large.

I know - I get so stuck in the process of work that I don't look at the whitespace.

I think you should take your time and put out quality fixes, instead of ignoring our coding guidelines and rushing out sloppy work. I am completely baffled why you still think this is okay. It's disrespectful to the team and the project IMO. What bothers me more is that you are not reviewing your own patches (or checking for compiler warnings, etc). 389 is not some amateur side project - it is an enterprise class Directory Server, and you should treat it as such.

I just want the quality of your code to match the quality of your designs. I really am trying to help you here (don't say I didn't try). So please take this all with a grain of salt.

You're absolutely right. I pride myself on the quality of my work, and I do invest a lot of time to try and make sure they are quality fixes. I spend a lot of time testing, and analysing to make sure that the code I (and everyone else) is writing works. I do understand the high profile nature of the project, and it does factor into my work. I obviously never meant disrespect, but I completely see where you are coming from with this.

Some of these points (re compiler warnings and such), was because I was missing the warnings on my own local setup (especially some of the ones that fedora CI was catching). Some of them were genuine mistakes (when I added make check to CI, I didn't realise there was still a compiler warning on something I had written in make check). I think that as a whole, we need to try and put out the best patches and code we can, but we also have to admit none of us are perfect. I make tons of mistakes - you make mistakes. We all do. As we make these mistakes we are learning from them over time. We need to rely on these tools and each other to make sure our code works - we all bring something to this project, that is needed.

I feel the root of this issue, especially the whitespace one is likely that it affects you and your editor setup pretty hard. For me, I don't even notice the difference - but for you it maybe it really affects your work. So for that, I'm sorry. I'll do better to try and make sure these fixes have better whitespacing and following this style guide (which I should follow - I wrote part of it).

I'm not trying to dismiss the issue - I want to just "fix it" so it's never an issue again (there is only so much time we can waste trying to fight this uphill battle of maintaining 3 code styles). This is why I'm suggesting that from now, when I/you/others find these whitespace issues, we discuss and submit reformats of the affected files (clang-format). As we go we'll probably find that we cover most of the code base with this.

So my suggestion here, is that I review the patch, identify the files that have the issue and submit a second patch where we have applied clang-format to the files. The biggest "risk" of this is in trying to rebase patches to older versions, so I really think the decision about this comes to you in the end.

Thanks :)

Ack this time, and please submit a second patch to fix the few lines that need fixing. Remember all these extra patches make backporting and cherry-picking fixes for downstream releases more difficult.

Ack this time, and please submit a second patch to fix the few lines that need fixing. Remember all these extra patches make backporting and cherry-picking fixes for downstream releases more difficult.

I thought I did fix the remaining whitespace issue? What did I miss?

commit 3133401199e9fd9d4de83400fa26e6a80e47baaf
To ssh://git@pagure.io/389-ds-base.git
54e4fca..19f676a master -> master

You have bad indentation here:

+void
+plugin_freeall(void)
+{
+   struct slapdplugin *plugin = NULL;
+   struct slapdplugin *plugin_next = NULL;
+   int type = 0;
+
+   /* look everywhere for other plugin functions with the plugin id */
+   for(type = 0; type < PLUGIN_LIST_GLOBAL_MAX; type++){
+       plugin = global_plugin_list[type];
+       while(plugin){
+           plugin_next = plugin->plg_next;
+            plugin_free(plugin);
+            plugin = plugin_next;
+       }
+   }
+}
+
 /*
  * Function: plugin_dependency_freeall
  *
@@ -1971,11 +1989,12 @@ plugin_dependency_freeall()
    while (iterp) {
        nextp = iterp->next;
        slapi_entry_free(iterp->e);
-       plugin_free(iterp->plugin);
+       /* plugin_free(iterp->plugin); */
        slapi_ch_free((void **)&iterp);
        iterp = nextp;
    }
    dep_plugin_entries = NULL;
+    plugin_freeall();
 }

Your last commit also generated a bunch of compiler warnings

../389-ds-base/ldap/servers/slapd/task.c: In function ‘task_import_add’:
../389-ds-base/ldap/servers/slapd/task.c:914:33: warning: assignment discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
                 nameFrombe_name = instance_name;
                                 ^
../389-ds-base/ldap/servers/slapd/back-ldbm/dblayer.c: In function ‘dblayer_start’:
../389-ds-base/ldap/servers/slapd/back-ldbm/dblayer.c:1270:19: warning: passing argument 1 of ‘slapi_ch_free’ from incompatible pointer type [-Wincompatible-pointer-types]
     slapi_ch_free(&priv->dblayer_home_directory);
                   ^
In file included from ../389-ds-base/ldap/servers/slapd/slapi-private.h:26:0,
                 from ../389-ds-base/ldap/servers/slapd/csngen.h:20,
                 from ../389-ds-base/ldap/servers/slapd/slap.h:113,
                 from ../389-ds-base/ldap/servers/slapd/back-ldbm/back-ldbm.h:88,
                 from ../389-ds-base/ldap/servers/slapd/back-ldbm/dblayer.c:63:
../389-ds-base/ldap/servers/slapd/slapi-plugin.h:5836:6: note: expected ‘void **’ but argument is of type ‘char **’
 void slapi_ch_free( void **ptr );
      ^~~~~~~~~~~~~
../389-ds-base/ldap/servers/slapd/back-ldbm/dblayer.c: In function ‘dblayer_restore_file_update’:
../389-ds-base/ldap/servers/slapd/back-ldbm/dblayer.c:7005:19: warning: passing argument 1 of ‘slapi_ch_free’ from incompatible pointer type [-Wincompatible-pointer-types]
     slapi_ch_free(&fname);
                   ^
In file included from ../389-ds-base/ldap/servers/slapd/slapi-private.h:26:0,
                 from ../389-ds-base/ldap/servers/slapd/csngen.h:20,
                 from ../389-ds-base/ldap/servers/slapd/slap.h:113,
                 from ../389-ds-base/ldap/servers/slapd/back-ldbm/back-ldbm.h:88,
                 from ../389-ds-base/ldap/servers/slapd/back-ldbm/dblayer.c:63:
../389-ds-base/ldap/servers/slapd/slapi-plugin.h:5836:6: note: expected ‘void **’ but argument is of type ‘char **’
 void slapi_ch_free( void **ptr );
      ^~~~~~~~~~~~~
../389-ds-base/ldap/servers/slapd/main.c: In function ‘slapd_exemode_ldif2db’:
../389-ds-base/ldap/servers/slapd/main.c:2040:28: warning: unused variable ‘slapdFrontendConfig’ [-Wunused-variable]
     slapdFrontendConfig_t *slapdFrontendConfig = getFrontendConfig();
                            ^~~~~~~~~~~~~~~~~~~
../389-ds-base/ldap/servers/slapd/main.c: In function ‘slapd_exemode_db2ldif’:
../389-ds-base/ldap/servers/slapd/main.c:2163:28: warning: unused variable ‘slapdFrontendConfig’ [-Wunused-variable]
     slapdFrontendConfig_t *slapdFrontendConfig = getFrontendConfig();
                            ^~~~~~~~~~~~~~~~~~~
../389-ds-base/ldap/servers/slapd/main.c: In function ‘slapd_exemode_db2index’:
../389-ds-base/ldap/servers/slapd/main.c:2379:28: warning: unused variable ‘slapdFrontendConfig’ [-Wunused-variable]
     slapdFrontendConfig_t *slapdFrontendConfig = getFrontendConfig();
                            ^~~~~~~~~~~~~~~~~~~
../389-ds-base/ldap/servers/slapd/main.c: In function ‘slapd_exemode_db2archive’:
../389-ds-base/ldap/servers/slapd/main.c:2468:25: warning: unused variable ‘slapdFrontendConfig’ [-Wunused-variable]
  slapdFrontendConfig_t *slapdFrontendConfig = getFrontendConfig();
                         ^~~~~~~~~~~~~~~~~~~
../389-ds-base/ldap/servers/slapd/main.c: In function ‘slapd_exemode_archive2db’:
../389-ds-base/ldap/servers/slapd/main.c:2515:25: warning: unused variable ‘slapdFrontendConfig’ [-Wunused-variable]
  slapdFrontendConfig_t *slapdFrontendConfig = getFrontendConfig();
                         ^~~~~~~~~~~~~~~~~~~
../389-ds-base/ldap/servers/slapd/main.c: In function ‘slapd_exemode_upgradedb’:
../389-ds-base/ldap/servers/slapd/main.c:2567:28: warning: unused variable ‘slapdFrontendConfig’ [-Wunused-variable]
     slapdFrontendConfig_t *slapdFrontendConfig = getFrontendConfig();
                            ^~~~~~~~~~~~~~~~~~~
../389-ds-base/ldap/servers/slapd/main.c: In function ‘slapd_exemode_upgradednformat’:
../389-ds-base/ldap/servers/slapd/main.c:2632:28: warning: unused variable ‘slapdFrontendConfig’ [-Wunused-variable]
     slapdFrontendConfig_t *slapdFrontendConfig = getFrontendConfig();
                            ^~~~~~~~~~~~~~~~~~~
At top level:
../389-ds-base/ldap/servers/slapd/main.c:94:1: warning: ‘nunc_stans_logging’ defined but not used [-Wunused-function]
 nunc_stans_logging(int severity, const char *format, va_list varg)
 ^~~~~~~~~~~~~~~~~~

I'll fix them now - I should of caught some of these during the review :(

e0289b7..120e818 master -> master

Your last commit also generated a bunch of compiler warnings

I was thinking this since I saw your conversation about the patch & push... I know it's disliked by members, but isn't it a good timing to consider the pull request style? Something like -- Using your own forked branch, the push triggers the build and the CI tests. Unless the proposed patch passes them, the patch won't be reviewed? Maybe, we could add a lint type of checking to the patch, as well? ;)

It looks to me the extra tasks for Mark is growing more than ever...>_<...

Your last commit also generated a bunch of compiler warnings

I was thinking this since I saw your conversation about the patch & push... I know it's disliked by members, but isn't it a good timing to consider the pull request style? Something like -- Using your own forked branch, the push triggers the build and the CI tests. Unless the proposed patch passes them, the patch won't be reviewed? Maybe, we could add a lint type of checking to the patch, as well? ;)
It looks to me the extra tasks for Mark is growing more than ever...>_<...

We might have to goto Pull Requests so we can block pushes that break builds. Currently master branch is broken from another ticket 49099 :( I'm spending the whole day trying to fix the server, meanwhile there is a customer issue I need to work on and this is blocking me :-/

Well, @vashirov has some CI stuff for PR worked out.

The reason I missed compiler warnings is probably because I have a fair few turned on and haven't finished fixing shadow variables yet so it was lost in the noise :(

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

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.