From c92c0243ca551b447fe8e5dedf7df371f623411f Mon Sep 17 00:00:00 2001 From: PengZheng Date: Tue, 25 Jul 2023 21:30:39 +0800 Subject: [PATCH] Eliminate GOTOs in discovery_zeroconf. --- .../src/DiscoveryZeroconfWatcherTestSuite.cc | 13 ++++ .../src/discovery_zeroconf_activator.c | 26 +++---- .../src/discovery_zeroconf_announcer.c | 74 ++++++++----------- .../src/discovery_zeroconf_announcer.h | 3 + .../src/discovery_zeroconf_watcher.c | 55 +++++++------- .../src/discovery_zeroconf_watcher.h | 3 + 6 files changed, 86 insertions(+), 88 deletions(-) diff --git a/bundles/remote_services/discovery_zeroconf/gtest/src/DiscoveryZeroconfWatcherTestSuite.cc b/bundles/remote_services/discovery_zeroconf/gtest/src/DiscoveryZeroconfWatcherTestSuite.cc index ccefa17be..54b968271 100644 --- a/bundles/remote_services/discovery_zeroconf/gtest/src/DiscoveryZeroconfWatcherTestSuite.cc +++ b/bundles/remote_services/discovery_zeroconf/gtest/src/DiscoveryZeroconfWatcherTestSuite.cc @@ -122,6 +122,19 @@ TEST_F(DiscoveryZeroconfWatcherTestSuite, CreateAndDestroyWatcher) { discoveryZeroconfWatcher_destroy(watcher); } +TEST_F(DiscoveryZeroconfWatcherTestSuite, Cleanup) { + celix_autoptr(discovery_zeroconf_watcher_t) watcher = nullptr; + celix_status_t status = discoveryZeroconfWatcher_create(ctx.get(), logHelper.get(), &watcher); + EXPECT_EQ(CELIX_SUCCESS, status); +} + +TEST_F(DiscoveryZeroconfWatcherTestSuite, StealPointer) { + celix_autoptr(discovery_zeroconf_watcher_t) watcher = nullptr; + celix_status_t status = discoveryZeroconfWatcher_create(ctx.get(), logHelper.get(), &watcher); + EXPECT_EQ(CELIX_SUCCESS, status); + discoveryZeroconfWatcher_destroy(celix_steal_ptr(watcher)); +} + TEST_F(DiscoveryZeroconfWatcherTestSuite, CreateWatcherFailed1) { discovery_zeroconf_watcher_t *watcher; celix_ei_expect_celix_bundleContext_getProperty((void*)&discoveryZeroconfWatcher_create, 0, nullptr); diff --git a/bundles/remote_services/discovery_zeroconf/src/discovery_zeroconf_activator.c b/bundles/remote_services/discovery_zeroconf/src/discovery_zeroconf_activator.c index ad5299414..239f0b1da 100644 --- a/bundles/remote_services/discovery_zeroconf/src/discovery_zeroconf_activator.c +++ b/bundles/remote_services/discovery_zeroconf/src/discovery_zeroconf_activator.c @@ -31,26 +31,24 @@ typedef struct discovery_zeroconf_activator { celix_status_t discoveryZeroconfActivator_start(discovery_zeroconf_activator_t *act, celix_bundle_context_t *ctx) { celix_status_t status = CELIX_SUCCESS; - act->logHelper = celix_logHelper_create(ctx,"celix_rsa_zeroconf_discovery"); - if (act->logHelper == NULL) { - status = CELIX_ENOMEM; - goto log_helper_err; + celix_autoptr(celix_log_helper_t) logger = celix_logHelper_create(ctx,"celix_rsa_zeroconf_discovery"); + if (logger == NULL) { + return CELIX_ENOMEM; } - status = discoveryZeroconfAnnouncer_create(ctx, act->logHelper, &act->announcer); + celix_autoptr(discovery_zeroconf_announcer_t) announcer = NULL; + status = discoveryZeroconfAnnouncer_create(ctx, logger, &announcer); if (status != CELIX_SUCCESS) { - goto announcer_err; + return status; } - status = discoveryZeroconfWatcher_create(ctx, act->logHelper, &act->watcher); + celix_autoptr(discovery_zeroconf_watcher_t) watcher = NULL; + status = discoveryZeroconfWatcher_create(ctx, logger, &watcher); if (status != CELIX_SUCCESS) { - goto watcher_err; + return status; } + act->watcher = celix_steal_ptr(watcher); + act->announcer = celix_steal_ptr(announcer); + act->logHelper = celix_steal_ptr(logger); return CELIX_SUCCESS; -watcher_err: - discoveryZeroconfAnnouncer_destroy(act->announcer); -announcer_err: - celix_logHelper_destroy(act->logHelper); -log_helper_err: - return status; } celix_status_t discoveryZeroconfActivator_stop(discovery_zeroconf_activator_t *act, celix_bundle_context_t *ctx) { diff --git a/bundles/remote_services/discovery_zeroconf/src/discovery_zeroconf_announcer.c b/bundles/remote_services/discovery_zeroconf/src/discovery_zeroconf_announcer.c index c297d174f..f14294414 100644 --- a/bundles/remote_services/discovery_zeroconf/src/discovery_zeroconf_announcer.c +++ b/bundles/remote_services/discovery_zeroconf/src/discovery_zeroconf_announcer.c @@ -31,6 +31,8 @@ #include "celix_types.h" #include "celix_errno.h" #include "celix_build_assert.h" +#include "celix_stdlib_cleanup.h" +#include "celix_unistd_cleanup.h" #include #include #include @@ -89,11 +91,10 @@ static void *discoveryZeroconfAnnouncer_refreshEndpointThread(void *data); celix_status_t discoveryZeroconfAnnouncer_create(celix_bundle_context_t *ctx, celix_log_helper_t *logHelper, discovery_zeroconf_announcer_t **announcerOut) { celix_status_t status = CELIX_SUCCESS; - discovery_zeroconf_announcer_t *announcer = (discovery_zeroconf_announcer_t *)calloc(1, sizeof(*announcer)); + celix_autofree discovery_zeroconf_announcer_t *announcer = (discovery_zeroconf_announcer_t *)calloc(1, sizeof(*announcer)); if (announcer == NULL) { celix_logHelper_fatal(logHelper, "Announcer: Failed to alloc announcer."); - status = CELIX_ENOMEM; - goto announcer_err; + return CELIX_ENOMEM; } announcer->ctx = ctx; announcer->logHelper = logHelper; @@ -101,27 +102,27 @@ celix_status_t discoveryZeroconfAnnouncer_create(celix_bundle_context_t *ctx, ce announcer->eventFd = eventfd(0, 0); if (announcer->eventFd < 0) { - status = CELIX_ERROR_MAKE(CELIX_FACILITY_CERRNO, errno); celix_logHelper_fatal(logHelper, "Announcer: Failed to open event fd, %d.", errno); - goto eventfd_err; + return CELIX_ERROR_MAKE(CELIX_FACILITY_CERRNO, errno); } + celix_auto(celix_fd_t) eventFd = announcer->eventFd; status = celixThreadMutex_create(&announcer->mutex, NULL); if (status != CELIX_SUCCESS) { celix_logHelper_fatal(logHelper, "Announcer: Failed to create mutex, %d.", status); - goto mutex_err; + return status; } + celix_autoptr(celix_thread_mutex_t) mutex = &announcer->mutex; - announcer->endpoints = celix_stringHashMap_create(); + celix_autoptr(celix_string_hash_map_t) endpoints = announcer->endpoints = celix_stringHashMap_create(); assert(announcer->endpoints != NULL); - announcer->revokedEndpoints = celix_arrayList_create(); + celix_autoptr(celix_array_list_t) revokedEndpoints = announcer->revokedEndpoints = celix_arrayList_create(); assert(announcer->revokedEndpoints != NULL); const char *fwUuid = celix_bundleContext_getProperty(ctx, OSGI_FRAMEWORK_FRAMEWORK_UUID, NULL); if (fwUuid == NULL || strlen(fwUuid) >= sizeof(announcer->fwUuid)) { - status = CELIX_BUNDLE_EXCEPTION; celix_logHelper_fatal(logHelper, "Announcer: Failed to get framework uuid."); - goto fw_uuid_err; + return CELIX_BUNDLE_EXCEPTION; } strcpy(announcer->fwUuid, fwUuid); @@ -141,35 +142,29 @@ celix_status_t discoveryZeroconfAnnouncer_create(celix_bundle_context_t *ctx, ce opt.svc = &announcer->epListener; announcer->epListenerSvcId = celix_bundleContext_registerServiceWithOptionsAsync(ctx, &opt); if (announcer->epListenerSvcId < 0) { - status = CELIX_BUNDLE_EXCEPTION; celix_logHelper_fatal(logHelper, "Announcer: Failed to register endpoint listener."); - goto ep_listener_err; + return CELIX_BUNDLE_EXCEPTION; } + celix_auto(celix_service_reg_t) epListenerSvcReg = { + .ctx = ctx, + .svcId = announcer->epListenerSvcId + }; announcer->running = true; status = celixThread_create(&announcer->refreshEPThread, NULL, discoveryZeroconfAnnouncer_refreshEndpointThread, announcer); if (status != CELIX_SUCCESS) { celix_logHelper_fatal(logHelper, "Announcer: Failed to create refreshing endpoint thread, %d.", status); - goto announce_ep_thread_err; + return status; } celixThread_setName(&announcer->refreshEPThread, "DiscAnnouncer"); - *announcerOut = announcer; + epListenerSvcReg.svcId = -1; + celix_steal_ptr(revokedEndpoints); + celix_steal_ptr(endpoints); + celix_steal_ptr(mutex); + celix_steal_fd(&eventFd); + *announcerOut = celix_steal_ptr(announcer); return CELIX_SUCCESS; -announce_ep_thread_err: - celix_bundleContext_unregisterServiceAsync(ctx, announcer->epListenerSvcId, NULL, NULL); - celix_bundleContext_waitForAsyncUnregistration(ctx, announcer->epListenerSvcId); -ep_listener_err: -fw_uuid_err: - celix_arrayList_destroy(announcer->revokedEndpoints); - celix_stringHashMap_destroy(announcer->endpoints); - celixThreadMutex_destroy(&announcer->mutex); -mutex_err: - close(announcer->eventFd); -eventfd_err: - free(announcer); -announcer_err: - return status; } void discoveryZeroconfAnnouncer_destroy(discovery_zeroconf_announcer_t *announcer) { @@ -229,7 +224,6 @@ static bool isLoopBackNetInterface(const char *ifName) { static celix_status_t discoveryZeroconfAnnouncer_endpointAdded(void *handle, endpoint_description_t *endpoint, char *matchedFilter) { (void)matchedFilter;//unused - celix_status_t status = CELIX_SUCCESS; discovery_zeroconf_announcer_t *announcer = (discovery_zeroconf_announcer_t *)handle; assert(announcer != NULL); if (endpointDescription_isInvalid(endpoint)) { @@ -239,11 +233,10 @@ static celix_status_t discoveryZeroconfAnnouncer_endpointAdded(void *handle, en celix_logHelper_info(announcer->logHelper, "Announcer: Add endpoint for %s(%s).", endpoint->serviceName, endpoint->id); - announce_endpoint_entry_t *entry = (announce_endpoint_entry_t *)calloc(1, sizeof(*entry)); + celix_autofree announce_endpoint_entry_t *entry = (announce_endpoint_entry_t *)calloc(1, sizeof(*entry)); if (entry == NULL) { celix_logHelper_error(announcer->logHelper, "Announcer: Failed to alloc endpoint entry."); - status = CELIX_ENOMEM; - goto endpoint_entry_err; + return CELIX_ENOMEM; } entry->registerRef = NULL; entry->announced = false; @@ -275,35 +268,28 @@ static celix_status_t discoveryZeroconfAnnouncer_endpointAdded(void *handle, en int bytes = snprintf(entry->serviceType, sizeof(entry->serviceType), DZC_SERVICE_PRIMARY_TYPE",%s", serviceSubType); if (bytes >= sizeof(entry->serviceType)) { celix_logHelper_error(announcer->logHelper, "Announcer: Please reduce the length of service type for %s.", serviceSubType); - status = CELIX_ILLEGAL_ARGUMENT; - goto service_type_err; + return CELIX_ILLEGAL_ARGUMENT; } } else { CELIX_BUILD_ASSERT(sizeof(entry->serviceType) >= sizeof(DZC_SERVICE_PRIMARY_TYPE)); strcpy(entry->serviceType, DZC_SERVICE_PRIMARY_TYPE); } - entry->properties = celix_properties_copy(endpoint->properties); + celix_autoptr(celix_properties_t) properties = entry->properties = celix_properties_copy(endpoint->properties); //Remove properties that remote service does not need celix_properties_unset(entry->properties, CELIX_RSA_NETWORK_INTERFACES); celix_properties_unset(entry->properties, DZC_SERVICE_TYPE_KEY); entry->serviceName = celix_properties_get(entry->properties, OSGI_FRAMEWORK_OBJECTCLASS, NULL); if (entry->serviceName == NULL) { - status = CELIX_ILLEGAL_ARGUMENT; celix_logHelper_error(announcer->logHelper,"Announcer: Invalid service."); - goto service_name_err; + return CELIX_ILLEGAL_ARGUMENT; } celixThreadMutex_lock(&announcer->mutex); - celix_stringHashMap_put(announcer->endpoints, endpoint->id, entry); + celix_stringHashMap_put(announcer->endpoints, endpoint->id, celix_steal_ptr(entry)); celixThreadMutex_unlock(&announcer->mutex); discoveryZeroconfAnnouncer_eventNotify(announcer); + celix_steal_ptr(properties); return CELIX_SUCCESS; -service_name_err: - celix_properties_destroy(entry->properties); -service_type_err: - free(entry); -endpoint_entry_err: - return status; } static celix_status_t discoveryZeroconfAnnouncer_endpointRemoved(void *handle, endpoint_description_t *endpoint, char *matchedFilter) { diff --git a/bundles/remote_services/discovery_zeroconf/src/discovery_zeroconf_announcer.h b/bundles/remote_services/discovery_zeroconf/src/discovery_zeroconf_announcer.h index 00f830cd4..f8f85923e 100644 --- a/bundles/remote_services/discovery_zeroconf/src/discovery_zeroconf_announcer.h +++ b/bundles/remote_services/discovery_zeroconf/src/discovery_zeroconf_announcer.h @@ -23,6 +23,7 @@ extern "C" { #endif +#include "celix_cleanup.h" #include "celix_log_helper.h" #include "celix_types.h" #include "celix_errno.h" @@ -33,6 +34,8 @@ celix_status_t discoveryZeroconfAnnouncer_create(celix_bundle_context_t *ctx, ce void discoveryZeroconfAnnouncer_destroy(discovery_zeroconf_announcer_t *announcer); +CELIX_DEFINE_AUTOPTR_CLEANUP_FUNC(discovery_zeroconf_announcer_t, discoveryZeroconfAnnouncer_destroy) + #ifdef __cplusplus } #endif diff --git a/bundles/remote_services/discovery_zeroconf/src/discovery_zeroconf_watcher.c b/bundles/remote_services/discovery_zeroconf/src/discovery_zeroconf_watcher.c index 1fcf997cd..70b21ba14 100644 --- a/bundles/remote_services/discovery_zeroconf/src/discovery_zeroconf_watcher.c +++ b/bundles/remote_services/discovery_zeroconf/src/discovery_zeroconf_watcher.c @@ -25,7 +25,9 @@ #include "celix_constants.h" #include "celix_long_hash_map.h" #include "celix_string_hash_map.h" +#include "celix_stdlib_cleanup.h" #include "celix_threads.h" +#include "celix_unistd_cleanup.h" #include "celix_utils.h" #include "celix_errno.h" #include @@ -89,11 +91,10 @@ static void *discoveryZeroconfWatcher_watchEPThread(void *data); celix_status_t discoveryZeroconfWatcher_create(celix_bundle_context_t *ctx, celix_log_helper_t *logHelper, discovery_zeroconf_watcher_t **watcherOut) { celix_status_t status = CELIX_SUCCESS; - discovery_zeroconf_watcher_t *watcher = (discovery_zeroconf_watcher_t *)calloc(1, sizeof(*watcher)); + celix_autofree discovery_zeroconf_watcher_t *watcher = (discovery_zeroconf_watcher_t *)calloc(1, sizeof(*watcher)); if (watcher == NULL) { celix_logHelper_fatal(logHelper, "Watcher: Failed to alloc watcher."); - status = CELIX_ENOMEM; - goto watcher_err; + return CELIX_ENOMEM; } watcher->logHelper = logHelper; watcher->ctx = ctx; @@ -101,33 +102,36 @@ celix_status_t discoveryZeroconfWatcher_create(celix_bundle_context_t *ctx, celi watcher->browseRef = NULL; watcher->eventFd = eventfd(0, 0); if (watcher->eventFd < 0) { - status = CELIX_ERROR_MAKE(CELIX_FACILITY_CERRNO, errno); celix_logHelper_fatal(logHelper, "Watcher: Failed to open event fd, %d.", errno); - goto event_fd_err; + return CELIX_ERROR_MAKE(CELIX_FACILITY_CERRNO, errno); } + celix_auto(celix_fd_t) eventFd = watcher->eventFd; const char *fwUuid = celix_bundleContext_getProperty(ctx, OSGI_FRAMEWORK_FRAMEWORK_UUID, NULL); if (fwUuid == NULL || strlen(fwUuid) >= sizeof(watcher->fwUuid)) { - status = CELIX_BUNDLE_EXCEPTION; celix_logHelper_fatal(logHelper, "Watcher: Failed to get framework uuid."); - goto fw_uuid_err; + return CELIX_BUNDLE_EXCEPTION; } strcpy(watcher->fwUuid, fwUuid); status = celixThreadMutex_create(&watcher->mutex, NULL); if (status != CELIX_SUCCESS) { celix_logHelper_fatal(logHelper, "Watcher: Failed to create mutex, %d.", status); - goto mutex_err; + return status; } + celix_autoptr(celix_thread_mutex_t) mutex = &watcher->mutex; celix_string_hash_map_create_options_t epOpts = CELIX_EMPTY_STRING_HASH_MAP_CREATE_OPTIONS; epOpts.storeKeysWeakly = true; - watcher->watchedEndpoints = celix_stringHashMap_createWithOptions(&epOpts); + celix_autoptr(celix_string_hash_map_t) watchedEndpoints = + watcher->watchedEndpoints = celix_stringHashMap_createWithOptions(&epOpts); assert(watcher->watchedEndpoints); celix_string_hash_map_create_options_t svcOpts = CELIX_EMPTY_STRING_HASH_MAP_CREATE_OPTIONS; - watcher->watchedServices = celix_stringHashMap_createWithOptions(&svcOpts); + celix_autoptr(celix_string_hash_map_t) watchedServices = + watcher->watchedServices = celix_stringHashMap_createWithOptions(&svcOpts); assert(watcher->watchedServices != NULL); - watcher->epls = celix_longHashMap_create(); + celix_autoptr(celix_long_hash_map_t) epls = + watcher->epls = celix_longHashMap_create(); assert(watcher->epls != NULL); celix_service_tracking_options_t opts = CELIX_EMPTY_SERVICE_TRACKING_OPTIONS; @@ -138,35 +142,26 @@ celix_status_t discoveryZeroconfWatcher_create(celix_bundle_context_t *ctx, celi opts.removeWithProperties = discoveryZeroconfWatcher_removeEPL; watcher->epListenerTrkId = celix_bundleContext_trackServicesWithOptionsAsync(ctx, &opts); if (watcher->epListenerTrkId < 0) { - status = CELIX_BUNDLE_EXCEPTION; celix_logHelper_fatal(logHelper, "Watcher: Failed to register endpoint listener service."); - goto epl_tracker_err; + return CELIX_BUNDLE_EXCEPTION; } watcher->running = true; status = celixThread_create(&watcher->watchEPThread,NULL, discoveryZeroconfWatcher_watchEPThread, watcher); if (status != CELIX_SUCCESS) { - goto thread_err; + celix_bundleContext_stopTrackerAsync(ctx, watcher->epListenerTrkId, NULL, NULL); + celix_bundleContext_waitForAsyncStopTracker(ctx, watcher->epListenerTrkId); + return status; } celixThread_setName(&watcher->watchEPThread, "DiscWatcher"); - *watcherOut = watcher; + celix_steal_ptr(epls); + celix_steal_ptr(watchedServices); + celix_steal_ptr(watchedEndpoints); + celix_steal_ptr(mutex); + celix_steal_fd(&eventFd); + *watcherOut = celix_steal_ptr(watcher); return CELIX_SUCCESS; -thread_err: - celix_bundleContext_stopTrackerAsync(ctx, watcher->epListenerTrkId, NULL, NULL); - celix_bundleContext_waitForAsyncStopTracker(ctx, watcher->epListenerTrkId); -epl_tracker_err: - celix_longHashMap_destroy(watcher->epls); - celix_stringHashMap_destroy(watcher->watchedServices); - celix_stringHashMap_destroy(watcher->watchedEndpoints); - celixThreadMutex_destroy(&watcher->mutex); -mutex_err: -fw_uuid_err: - close(watcher->eventFd); -event_fd_err: - free(watcher); -watcher_err: - return status; } void discoveryZeroconfWatcher_destroy(discovery_zeroconf_watcher_t *watcher) { diff --git a/bundles/remote_services/discovery_zeroconf/src/discovery_zeroconf_watcher.h b/bundles/remote_services/discovery_zeroconf/src/discovery_zeroconf_watcher.h index 7d3f60e76..ee9c97d15 100644 --- a/bundles/remote_services/discovery_zeroconf/src/discovery_zeroconf_watcher.h +++ b/bundles/remote_services/discovery_zeroconf/src/discovery_zeroconf_watcher.h @@ -23,6 +23,7 @@ extern "C" { #endif +#include "celix_cleanup.h" #include "celix_log_helper.h" #include "celix_types.h" #include "celix_errno.h" @@ -33,6 +34,8 @@ celix_status_t discoveryZeroconfWatcher_create(celix_bundle_context_t *ctx, celi void discoveryZeroconfWatcher_destroy(discovery_zeroconf_watcher_t *watcher); +CELIX_DEFINE_AUTOPTR_CLEANUP_FUNC(discovery_zeroconf_watcher_t, discoveryZeroconfWatcher_destroy) + #ifdef __cplusplus } #endif