[RFC] Scope-based Resource Management for Celix

[PengZheng]

This PR aims to add support for SBRM, following a recent Linux kernel patch set (#574). However, IMO, the kernel's approach is too complicated for general user-space application development. Thus a modified/simplified glib's approach is adopted.

It is still a work-in-progress, which lacks:

• [x] Documentation
• [x] Sufficient in-project usage to gain experience: all GOTOs should be eliminated.
• [x] Tests
• [x] Best practices documented in our Coding Convention

This PR should not be merged until the first three have been done.

It also introduces CELIX_UNIQUE_ID to fix #593.

[codecov-commenter]

Codecov Report

Merging #591 (feed249) into master (d214522) will increase coverage by 0.00%. The diff coverage is 95.07%.

@@           Coverage Diff            @@
##           master     #591    +/-   ##
========================================
  Coverage   78.51%   78.52%            
========================================
  Files         234      253    +19     
  Lines       35389    35256   -133     
========================================
- Hits        27786    27684   -102     
+ Misses       7603     7572    -31     

Files Changed	Coverage Δ
...les/pubsub/pubsub_admin_tcp/src/pubsub_tcp_admin.c	50.50% <0.00%> (ø)
...s/pubsub/pubsub_admin_tcp/src/pubsub_tcp_handler.c	78.31% <0.00%> (ø)
...sub/pubsub_admin_tcp/src/pubsub_tcp_topic_sender.c	84.01% <ø> (ø)
...ubsub/pubsub_admin_udp_mc/src/pubsub_udpmc_admin.c	41.62% <0.00%> (ø)
...ubsub_admin_websocket/src/pubsub_websocket_admin.c	42.47% <0.00%> (ø)
...lizer_avrobin/src/pubsub_avrobin_serializer_impl.c	5.63% <0.00%> (ø)
...dles/pubsub/pubsub_spi/src/pubsub_endpoint_match.c	22.14% <0.00%> (ø)
libs/framework/src/bundle_archive.c	100.00% <ø> (ø)
libs/framework/src/bundle_revision.c	100.00% <ø> (ø)
libs/framework/src/celix_scheduled_event.c	100.00% <ø> (ø)
... and 57 more

... and 2 files with indirect coverage changes

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[pnoltes]

Nice work!. Very interesting and welcome addition. For me 1 of the 3 major issues with C is no support for "scoped based" cleanup and this somewhat solves that (the other ones are no type safe containers and no easy way to do white-box code reuse (e.g. inheritance/polymorphism).

And very nice that the user can do this - if I am correct - with 3 macros celxi_auto, celix_autoptr and celix_autofree.

I want to review this more thoroughly, but do need some time to do this.

[PengZheng]

I want to review this more thoroughly, but do need some time to do this.

Please do it and join the experiment! I'm still experimenting by eliminating GOTOs in rsa_shm.

I encountered the "destructor with extra arguments" issue mentioned by this LWN comment: https://lwn.net/Articles/934838/

The current solution is like this: https://github.com/apache/celix/pull/591/files#diff-9c5bdc83321bdc259803d92847d9f23132c5b8321f24fe625ca282cfabb9cd9eR123

with its usage: https://github.com/apache/celix/pull/591/files#diff-9be07887dc8325bf3802ded6a943d9a42b1dae30ea4c092013b78cf19615f2e9R103

[PengZheng]

Most of GOTOs are eliminated by this PR. But there are some exceptions, one of which is celix_bundleArchive_create:

store_prop_failed:
revision_failed:
dir_failed:
    if (!isSystemBundle) {
        celix_utils_deleteDirectory(archive->archiveRoot, NULL);
    }
init_failed:
    bundleArchive_destroy(archive);
calloc_failed:
    framework_logIfError(fw->logger, status, error, "Could not create archive.");
    return status;

It does not really produce anything reusable by wrapping celix_utils_deleteDirectory. IMHO, it should be wrapped in an ad hoc lambda expression, which we don't have in C. The best we have is Apple "blocks" extension, which GCC does not support. GCC does have nested functions, which unfortunately require executable stack, which is a security hole.

The most important lesson I learned along the way is that we shall NEVER mix the usage of GOTOs and auto variables. The following is very dangerous, since cleanup function of an uninitialized auto variable will be triggered by goto and a crash will follow:

    if(error) {
        goto exit;
    }
    celix_autofree char* buf = malloc(4);
    // omitted
exit:
    return 1;

Static code analysis tools (e.g. CLion) do a great job of reporting the above issue.

[pnoltes]

Most of GOTOs are eliminated by this PR. But there are some exceptions, one of which is celix_bundleArchive_create:

store_prop_failed:
revision_failed:
dir_failed:
    if (!isSystemBundle) {
        celix_utils_deleteDirectory(archive->archiveRoot, NULL);
    }
init_failed:
    bundleArchive_destroy(archive);
calloc_failed:
    framework_logIfError(fw->logger, status, error, "Could not create archive.");
    return status;

It does not really produce anything reusable by wrapping celix_utils_deleteDirectory. IMHO, it should be wrapped in an ad hoc lambda expression, which we don't have in C. The best we have is Apple "blocks" extension, which GCC does not support. GCC does have nested functions, which unfortunately require executable stack, which is a security hole.

The most important lesson I learned along the way is that we shall NEVER mix the usage of GOTOs and auto variables. The following is very dangerous, since cleanup function of an uninitialized auto variable will be triggered by goto and a crash will follow:

    if(error) {
        goto exit;
    }
    celix_autofree char* buf = malloc(4);
    // omitted
exit:
    return 1;

Static code analysis tools (e.g. CLion) do a great job of reporting the above issue.

Good to known and something to focus on during review. Any idea if a goto / cleanup combination always gives issues or is it also possible this goes unnoticed during compilation and testing? If the former is the case, that this will (generally) be solved before a pull request is made.

[PengZheng]

Any idea if a goto / cleanup combination always gives issues or is it also possible this goes unnoticed during compilation and testing?

Goto/cleanup combination is not always an issue. For exmaple, there is actually nothing wrong with the following:

    celix_autofree char* buf = malloc(4);
    if(error) {
        goto exit;
    }  
    // omitted
    return 0;
exit:
    return 1;

The caveat is that declaration of auto variables should NEVER appear AFTER goto statement. It is just a rule of thumb that we shall avoid mixed usage of GOTOs and auto variables to ease code review.

clang will catch this issue:

#include <stdio.h>
#include <malloc.h>

static __attribute__((unused)) inline void celix_autoptr_cleanup_generic_free(void* p) {
    void** pp = (void**)p;
    free(*pp);
}

int main() {
    goto end;
    __attribute__((unused)) __attribute__((cleanup(celix_autoptr_cleanup_generic_free))) void* ptr = malloc(13);
    end:
    return 0;
}

/usr/bin/clang-11   -g -std=gnu11 -fcolor-diagnostics -MD -MT CMakeFiles/tst.dir/main.c.o -MF CMakeFiles/tst.dir/main.c.o.d -o CMakeFiles/tst.dir/main.c.o -c /home/peng/Downloads/tst/main.c
/home/peng/Downloads/tst/main.c:10:5: error: cannot jump from this goto statement to its label
    goto end;
    ^
/home/peng/Downloads/tst/main.c:11:96: note: jump bypasses initialization of variable with __attribute__((cleanup))
    __attribute__((unused)) __attribute__((cleanup(celix_autoptr_cleanup_generic_free))) void* ptr = malloc(13);
                                                                                               ^
1 error generated.
ninja: build stopped: subcommand failed.

On the other hand, gcc lacks this capability now: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91951

[PengZheng]

The cleanup macros are IMO only really valuable during object creation / initiation and RAII-based guard object. @PengZheng do you agree?

Yes, this should be made explicit in our documentation.

C Objects should define a cleanup function and

Provide a celix_create or celix_init function. The latter is used when no memory allocation is needed. The create function returns a pointer (possible NULL) and the _init returns a celix_statust or a struct by value. Provide a celixdestroy or celixdeinit function. The latter is used when no memory needs to be deallocated. If the structure "guards" a condition for a scope (lock guard, service registration guard, etc) it should use the naming convention celix_guard_t (so celix_file_guard_t, or celix_threadMutex_guard_t

Naming conventions really matter. The third one is conceptually consistent and is far better than the one I've used before.

There are two usage patterns with different semantics:

1. Modify existing types to add autocleanup. Examples include celix_auto(celix_fd_t) /celix_autoptr(celix_string_hash_map_t)
2. Introduce new type to perform specific actions (celix_mutex_lock_guard_t) or to capture cleanup context (celix_service_registration_guard_t)

It is the second pattern to which the naming convention applies. I have updated all such usages to the suggested naming convention.