PengZheng commented on code in PR #751:
URL: https://github.com/apache/celix/pull/751#discussion_r1632631225
##########
libs/framework/src/framework.c:
##########
@@ -2345,61 +2362,73 @@ static bool
celix_framework_updateBundleInternal(celix_framework_t *fw, long bnd
}
celix_status_t celix_framework_updateBundleEntry(celix_framework_t* framework,
-
celix_framework_bundle_entry_t* bndEntry,
+ celix_bundle_entry_t*
bndEntry,
const char* updatedBundleUrl)
{
celix_status_t status = CELIX_SUCCESS;
long bundleId = bndEntry->bndId;
- const char* errMsg;
fw_log(framework->logger, CELIX_LOG_LEVEL_DEBUG, "Updating bundle %s",
celix_bundle_getSymbolicName(bndEntry->bnd));
celix_bundle_state_e bndState = celix_bundle_getState(bndEntry->bnd);
- char *location = celix_bundle_getLocation(bndEntry->bnd);
- do {
- // lock to reuse the bundle id
- celixThreadMutex_lock(&framework->installLock);
- if (updatedBundleUrl == NULL) {
- updatedBundleUrl = location;
- } else if (strcmp(location, updatedBundleUrl) != 0) {
- long alreadyExistingBndId =
celix_bundleCache_findBundleIdForLocation(framework->cache, updatedBundleUrl);
- if (alreadyExistingBndId != -1 && alreadyExistingBndId !=
bundleId) {
- errMsg = "specified bundle location exists in cache with a
different id";
- celix_framework_bundleEntry_decreaseUseCount(bndEntry);
- celixThreadMutex_unlock(&framework->installLock);
- status = CELIX_ILLEGAL_STATE;
- break;
- }
-
celix_bundleArchive_invalidateCache(celix_bundle_getArchive(bndEntry->bnd));
- }
- status = celix_framework_uninstallBundleEntryImpl(framework, bndEntry,
false);
+ celix_autofree char* location = celix_bundle_getLocation(bndEntry->bnd);
+
+ // lock to reuse the bundle id
+ celix_auto(celix_mutex_lock_guard_t) lck =
celixMutexLockGuard_init(&framework->installLock);
+ if (updatedBundleUrl == NULL) {
+ updatedBundleUrl = location;
+ } else if (strcmp(location, updatedBundleUrl) != 0) {
+ long existingBndId;
+ status = celix_bundleCache_findBundleIdForLocation(framework->cache,
updatedBundleUrl, &existingBndId);
if (status != CELIX_SUCCESS) {
Review Comment:
Note that this error can never happen in practice:
`cache->locationToBundleIdLookupMapLoaded` must be true, because `bndEntry` has
already been installed.
##########
libs/framework/src/framework.c:
##########
@@ -624,63 +649,60 @@ bool
celix_framework_getConfigPropertyAsBool(celix_framework_t* framework, const
static celix_status_t
celix_framework_installBundleInternalImpl(celix_framework_t* framework, const
char* bndLoc, long* bndId) {
- celix_status_t status = CELIX_SUCCESS;
- celix_bundle_t* bundle = NULL;
- long id = -1L;
-
- bundle_state_e state = CELIX_BUNDLE_STATE_UNKNOWN;
-
bool valid = celix_framework_utils_isBundleUrlValid(framework, bndLoc,
false);
if (!valid) {
return CELIX_FILE_IO_EXCEPTION;
}
- //increase use count of framework bundle to prevent a stop.
- celix_framework_bundle_entry_t *fwBundleEntry =
celix_framework_bundleEntry_getBundleEntryAndIncreaseUseCount(framework,
-
framework->bundleId);
- status = CELIX_DO_IF(status, bundle_getState(framework->bundle, &state));
- if (status == CELIX_SUCCESS) {
- if (state == CELIX_BUNDLE_STATE_STOPPING || state ==
CELIX_BUNDLE_STATE_UNINSTALLED) {
- fw_log(framework->logger, CELIX_LOG_LEVEL_INFO, "The framework is
being shutdown");
- status = CELIX_FRAMEWORK_SHUTDOWN;
- }
+ // increase use count of framework bundle to prevent a stop.
+ celix_bundle_entry_t* fwBundleEntry =
+
celix_framework_bundleEntry_getBundleEntryAndIncreaseUseCount(framework,
framework->bundleId);
+ celix_auto(celix_bundle_entry_use_guard_t) fwEntryGuard =
celix_bundleEntryUseGuard_init(fwBundleEntry);
+
+ celix_bundle_state_e bndState = celix_bundle_getState(framework->bundle);
+ if (bndState == CELIX_BUNDLE_STATE_STOPPING || bndState ==
CELIX_BUNDLE_STATE_UNINSTALLED) {
+ fw_log(framework->logger, CELIX_LOG_LEVEL_INFO, "The framework is
being shutdown");
+ return CELIX_FRAMEWORK_SHUTDOWN;
}
- if (status == CELIX_SUCCESS) {
- if (*bndId == -1L) {
- id = framework_getBundle(framework, bndLoc);
- if (id != -1L) {
- celix_framework_bundleEntry_decreaseUseCount(fwBundleEntry);
- *bndId = id;
- return CELIX_SUCCESS;
- }
- long alreadyExistingBndId =
celix_bundleCache_findBundleIdForLocation(framework->cache, bndLoc);
- id = alreadyExistingBndId == -1 ?
framework_getNextBundleId(framework) : alreadyExistingBndId;
- } else {
- id = *bndId;
+ long id;
+ if (*bndId == -1L) {
+ id = framework_getBundle(framework, bndLoc);
+ if (id != -1L) {
+ *bndId = id;
+ return CELIX_SUCCESS;
}
- bundle_archive_t* archive = NULL;
- status = CELIX_DO_IF(status,
celix_bundleCache_createArchive(framework->cache, id, bndLoc, &archive));
- status = CELIX_DO_IF(status, celix_bundle_createFromArchive(framework,
archive, &bundle));
- if (status == CELIX_SUCCESS) {
- celix_framework_bundle_entry_t *bEntry =
fw_bundleEntry_create(bundle);
- celix_framework_bundleEntry_increaseUseCount(bEntry);
- celixThreadMutex_lock(&framework->installedBundles.mutex);
- celix_arrayList_add(framework->installedBundles.entries, bEntry);
- celixThreadMutex_unlock(&framework->installedBundles.mutex);
- fw_fireBundleEvent(framework,
OSGI_FRAMEWORK_BUNDLE_EVENT_INSTALLED, bEntry);
- celix_framework_bundleEntry_decreaseUseCount(bEntry);
+ long alreadyExistingBndId;
+ celix_status_t status =
+ celix_bundleCache_findBundleIdForLocation(framework->cache,
bndLoc, &alreadyExistingBndId);
+ if (status != CELIX_SUCCESS) {
+ fw_logCode(framework->logger, CELIX_LOG_LEVEL_ERROR, status,
"Could not install bundle");
+ return status;
}
+ id = alreadyExistingBndId == -1 ? framework_getNextBundleId(framework)
: alreadyExistingBndId;
+ } else {
+ id = *bndId;
}
- if (status == CELIX_SUCCESS) {
- *bndId = id;
- } else {
- fw_logCode(framework->logger, CELIX_LOG_LEVEL_ERROR, status, "Could
not install bundle");
+ bundle_archive_t* archive = NULL;
+ celix_bundle_t* bundle = NULL;
+ celix_status_t status = celix_bundleCache_createArchive(framework->cache,
id, bndLoc, &archive);
+ status = CELIX_DO_IF(status, celix_bundle_createFromArchive(framework,
archive, &bundle));
+ if (status != CELIX_SUCCESS) {
+ celix_bundleArchive_destroy(archive);
Review Comment:
This is not enough. I think `celix_bundleCache_destroyArchive` should be
used instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
