PengZheng commented on code in PR #476:
URL: https://github.com/apache/celix/pull/476#discussion_r1140162136


##########
libs/framework/src/bundle_archive.c:
##########
@@ -28,644 +28,451 @@
 #include <dirent.h>
 #include <unistd.h>
 #include <string.h>
+#include <assert.h>
 
 #include "celix_constants.h"
 #include "celix_utils_api.h"
 #include "linked_list_iterator.h"
 #include "framework_private.h"
 #include "celix_file_utils.h"
 #include "bundle_revision_private.h"
+#include "celix_framework_utils_private.h"
 
+
+celix_status_t celix_bundleArchive_getLastModifiedInternal(bundle_archive_pt 
archive, struct timespec *lastModified);
+
+/**
+ * The bundle archive which is used to store the bundle data and can be reused 
when a framework is restarted.
+ * The lifecycle of a bundle archive is coupled to the lifecycle of the bundle 
that is created from the archive.
+ *
+ * @note The bundle archive is thread safe.
+ */
 struct bundleArchive {
+    //initialed during creation and immutable
        celix_framework_t* fw;
        long id;
-       char * location;
-       DIR *archiveRootDir;
-       char * archiveRoot;
-       linked_list_pt revisions;
-       long refreshCount;
-       time_t lastModified;
-
-       bundle_state_e persistentState;
-};
+       char *archiveRoot;
+    char *savedBundleStatePropertiesPath;
+    char* storeRoot;
+    bool isSystemBundle;
+    char* bundleSymbolicName; //read from the manifest
+    char* bundleVersion; //read from the manifest
 
-static celix_status_t bundleArchive_getRevisionLocation(bundle_archive_pt 
archive, long revNr, char **revision_location);
-static celix_status_t bundleArchive_setRevisionLocation(bundle_archive_pt 
archive, const char * location, long revNr);
 
-static celix_status_t bundleArchive_initialize(bundle_archive_pt archive);
+    celix_thread_mutex_t lock; //protects below and saving of bundle state 
properties
 
-static celix_status_t 
bundleArchive_createRevisionFromLocation(bundle_archive_pt archive, const char 
*location, const char *inputFile, long revNr, bundle_revision_pt 
*bundle_revision);
-static celix_status_t bundleArchive_reviseInternal(bundle_archive_pt archive, 
bool isReload, long revNr, const char * location, const char *inputFile);
+    char* location;
+    char* currentRevisionRoot;
+    celix_array_list_t* revisions; //list of bundle_revision_t*
 
-static celix_status_t bundleArchive_readLastModified(bundle_archive_pt 
archive, time_t *time);
-static celix_status_t bundleArchive_writeLastModified(bundle_archive_pt 
archive);
+    //bundle cache state info
+       long revisionNr;
 
-celix_status_t bundleArchive_createSystemBundleArchive(celix_framework_t* fw, 
bundle_archive_pt *bundle_archive) {
-       celix_status_t status = CELIX_SUCCESS;
-       char *error = NULL;
-       bundle_archive_pt archive = NULL;
-
-       if (*bundle_archive != NULL) {
-               status = CELIX_ILLEGAL_ARGUMENT;
-               error = "Missing required arguments and/or incorrect values";
-       } else {
-               archive = (bundle_archive_pt) calloc(1,sizeof(*archive));
-               if (archive == NULL) {
-                       status = CELIX_ENOMEM;
-               } else {
-                       status = linkedList_create(&archive->revisions);
-                       if (status == CELIX_SUCCESS) {
-                               archive->fw = fw;
-                               archive->id = CELIX_FRAMEWORK_BUNDLE_ID;
-                               archive->location = strdup("System Bundle");
-                               archive->archiveRoot = NULL;
-                               archive->archiveRootDir = NULL;
-                               archive->refreshCount = -1;
-                               archive->persistentState = 
CELIX_BUNDLE_STATE_UNKNOWN;
-                               time(&archive->lastModified);
-
-                               *bundle_archive = archive;
-                       }
-               }
-       }
-
-       if(status != CELIX_SUCCESS && archive != NULL){
-               bundleArchive_destroy(archive);
-       }
-
-       framework_logIfError(fw->logger, status, error, "Could not create 
archive");
+    //bundle cache state in properties form
+    celix_properties_t* bundleStateProperties;
+};
 
-       return status;
+static void 
celix_bundleArchive_updateAndStoreBundleStateProperties(bundle_archive_pt 
archive) {
+    celixThreadMutex_lock(&archive->lock);
+    //set/update bundle cache state properties
+    celix_properties_setLong(archive->bundleStateProperties, 
CELIX_BUNDLE_ARCHIVE_BUNDLE_ID_PROPERTY_NAME, archive->id);
+    celix_properties_set(archive->bundleStateProperties, 
CELIX_BUNDLE_ARCHIVE_LOCATION_PROPERTY_NAME, archive->location);
+    celix_properties_set(archive->bundleStateProperties, 
CELIX_BUNDLE_ARCHIVE_SYMBOLIC_NAME_PROPERTY_NAME, archive->bundleSymbolicName);
+    celix_properties_set(archive->bundleStateProperties, 
CELIX_BUNDLE_ARCHIVE_VERSION_PROPERTY_NAME, archive->bundleVersion);
+    celix_properties_setLong(archive->bundleStateProperties, 
CELIX_BUNDLE_ARCHIVE_REVISION_PROPERTY_NAME, archive->revisionNr);
+
+    //save bundle cache state properties
+    celix_properties_store(archive->bundleStateProperties, 
archive->savedBundleStatePropertiesPath, "Bundle State Properties");
+    celixThreadMutex_unlock(&archive->lock);
 }
 
-celix_status_t bundleArchive_create(celix_framework_t* fw, const char 
*archiveRoot, long id, const char * location, const char *inputFile, 
bundle_archive_pt *bundle_archive) {
-       celix_status_t status = CELIX_SUCCESS;
-       char *error = NULL;
-       bundle_archive_pt archive = NULL;
-
-       if (*bundle_archive != NULL) {
-               status = CELIX_ILLEGAL_ARGUMENT;
-               error = "bundle_archive_pt must be NULL";
-       } else {
-               archive = (bundle_archive_pt) calloc(1,sizeof(*archive));
-               if (archive == NULL) {
-                       status = CELIX_ENOMEM;
-               } else {
-                       status = linkedList_create(&archive->revisions);
-                       if (status == CELIX_SUCCESS) {
-                               archive->fw = fw;
-                               archive->id = id;
-                               archive->location = strdup(location);
-                               archive->archiveRootDir = NULL;
-                               archive->archiveRoot = strdup(archiveRoot);
-                               archive->refreshCount = -1;
-                               time(&archive->lastModified);
-
-                               status = bundleArchive_initialize(archive);
-                               if (status == CELIX_SUCCESS) {
-                                       status = bundleArchive_revise(archive, 
location, inputFile);
-
-                                       if (status == CELIX_SUCCESS) {
-                                               *bundle_archive = archive;
-                                       }
-                                       else{
-                                               
bundleArchive_closeAndDelete(archive);
-                                       }
-                               }
-                       }
-               }
-       }
-
-       if(status != CELIX_SUCCESS && archive != NULL){
-               bundleArchive_destroy(archive);
-       }
+celix_status_t celix_bundleArchive_extractBundle(
+        bundle_archive_t* archive,
+        const char* revisionRoot,
+        const char* bundleUrl,
+        const struct timespec* revisionModificationTime) {
+    celix_status_t status = CELIX_SUCCESS;
+    bool extractBundle = true;
+
+    //check if bundle location is newer than current revision
+    if (celix_utils_fileExists(revisionRoot)) {
+        extractBundle = 
celix_framework_utils_isBundleUrlNewerThan(archive->fw, bundleUrl, 
revisionModificationTime);
+    }
+
+    if (!extractBundle) {
+        fw_log(archive->fw->logger, CELIX_LOG_LEVEL_TRACE, "Bundle archive %s 
is up to date, no need to extract bundle.", bundleUrl);
+        return status;
+    }
+
+    /*
+     * Note always remove the current revision dir. This is needed to remove 
files that are not present
+     * in the new bundle zip, but it seems this is also needed to ensure that 
the lib files get a new inode.
+     * If dlopen/dlsym is used with newer files, but with the same inode 
already used in dlopen/dlsym this leads to
+     * segfaults.
+     */
+    const char* error;
+    status = celix_utils_deleteDirectory(revisionRoot, &error);
+    if (status != CELIX_SUCCESS) {
+        fw_logCode(archive->fw->logger, CELIX_LOG_LEVEL_ERROR, status, "Failed 
to remove existing bundle archive revision directory '%s': %s", revisionRoot, 
error);
+        return status;
+    }
+
+    status = celix_framework_utils_extractBundle(archive->fw, bundleUrl, 
revisionRoot);
+    if (status != CELIX_SUCCESS) {
+        fw_log(archive->fw->logger, CELIX_LOG_LEVEL_ERROR, "Failed to 
initialize archive. Failed to extract bundle zip to revision directory.");
+        return status;
+    }
+    return status;
+}
 
-       framework_logIfError(fw->logger, status, error, "Could not create 
archive");
+/**
+ * Initialize archive by creating the bundle cache directory, optionally 
extracting the bundle from the bundle file,
+ * reading the bundle state properties, reading the bundle manifest and 
updating the bundle state properties.
+ */
+celix_status_t celix_bundleArchive_createCacheDirectory(bundle_archive_pt 
archive, manifest_pt* manifestOut) {
+    if (celix_utils_fileExists(archive->archiveRoot)) {
+        fw_log(archive->fw->logger, CELIX_LOG_LEVEL_TRACE, "Bundle archive 
root for bundle id %li already exists.",
+               archive->id);
+    }
+
+    //create archive root
+    const char* errorStr = NULL;
+    celix_status_t status = celix_utils_createDirectory(archive->archiveRoot, 
false, &errorStr);
+    if (status != CELIX_SUCCESS) {
+        fw_log(archive->fw->logger, CELIX_LOG_LEVEL_ERROR, "Failed to 
initialize archive. Failed to create bundle root archive dir: %s", errorStr);
+        return status;
+    }
+
+    //create store directory
+    int rc = asprintf(&archive->storeRoot, "%s/%s", archive->archiveRoot, 
CELIX_BUNDLE_ARCHIVE_STORE_DIRECTORY_NAME);
+    if (rc < 0) {
+        fw_log(archive->fw->logger, CELIX_LOG_LEVEL_ERROR, "Failed to 
initialize archive. Failed to create bundle store dir.");
+        return CELIX_ENOMEM;
+    }
+    status = celix_utils_createDirectory(archive->storeRoot, false, &errorStr);
+    if (status != CELIX_SUCCESS) {
+        fw_log(archive->fw->logger, CELIX_LOG_LEVEL_ERROR, "Failed to 
initialize archive. Failed to create bundle store dir: %s", errorStr);
+        return status;
+    }
+
+    //create bundle revision directory
+    rc = asprintf(&archive->currentRevisionRoot, 
CELIX_BUNDLE_ARCHIVE_REVISION_DIRECTORY_NAME_FORMAT, archive->archiveRoot, 
archive->revisionNr);
+    if (rc < 0) {
+        fw_log(archive->fw->logger, CELIX_LOG_LEVEL_ERROR, "Failed to 
initialize archive. Failed to create bundle revision dir.");
+        return CELIX_ENOMEM;
+    }
+
+    status = celix_utils_createDirectory(archive->currentRevisionRoot, false, 
&errorStr);
+    if (status != CELIX_SUCCESS) {
+        fw_log(archive->fw->logger, CELIX_LOG_LEVEL_ERROR, "Failed to 
initialize archive. Failed to create bundle revision dir: %s", errorStr);
+        return status;
+    }
+
+    //get revision mod time;
+    struct timespec revisionMod;
+    status = celix_bundleArchive_getLastModifiedInternal(archive, 
&revisionMod);
+    if (status != CELIX_SUCCESS) {
+        fw_logCode(archive->fw->logger, CELIX_LOG_LEVEL_ERROR, status, "Failed 
to get last modified time for bundle archive revision directory '%s'", 
archive->currentRevisionRoot);
+        return status;
+    }
+
+    //extract bundle zip to revision directory
+    status = celix_bundleArchive_extractBundle(archive, 
archive->currentRevisionRoot, archive->location, &revisionMod);
+    if (status != CELIX_SUCCESS) {
+        fw_log(archive->fw->logger, CELIX_LOG_LEVEL_ERROR, "Failed to 
initialize archive. Failed to extract bundle.");
+        return status;
+    }
+
+    //read manifest from extracted bundle zip
+    *manifestOut = NULL;
+    char pathBuffer[512];
+    char* manifestPath = celix_utils_writeOrCreateString(pathBuffer, 
sizeof(pathBuffer), "%s/%s", archive->currentRevisionRoot, 
CELIX_BUNDLE_MANIFEST_REL_PATH);
+    status = manifest_createFromFile(manifestPath, manifestOut);
+    celix_utils_freeStringIfNotEqual(pathBuffer, manifestPath);
+    if (status != CELIX_SUCCESS) {
+        fw_log(archive->fw->logger, CELIX_LOG_LEVEL_ERROR, "Failed to 
initialize archive. Cannot read manifest.");
+        manifest_destroy(*manifestOut);

Review Comment:
   ```suggestion
   ```
   
   `manifest_createFromFile` never leaks manifest in error condition.



-- 
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: dev-unsubscr...@celix.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Reply via email to