pnoltes commented on code in PR #616:
URL: https://github.com/apache/celix/pull/616#discussion_r1298287076
##########
libs/framework/src/manifest.c:
##########
@@ -24,271 +24,291 @@
* \copyright Apache License, Version 2.0
*/
-#include <stdio.h>
+#include <limits.h>
#include <stdlib.h>
#include <string.h>
#include <stdbool.h>
+#include "celix_err.h"
+#include "celix_errno.h"
+#include "celix_stdio_cleanup.h"
+#include "celix_stdlib_cleanup.h"
+#include "celix_utils.h"
#include "manifest.h"
#include "utils.h"
-#include "celix_log.h"
-
-int fpeek(FILE *stream);
-
-static celix_status_t manifest_readAttributes(manifest_pt manifest,
properties_pt properties, FILE *file);
celix_status_t manifest_create(manifest_pt *manifest) {
- celix_status_t status = CELIX_SUCCESS;
-
- *manifest = malloc(sizeof(**manifest));
- if (!*manifest) {
- status = CELIX_ENOMEM;
- } else {
- (*manifest)->mainAttributes = properties_create();
- (*manifest)->attributes = hashMap_create(utils_stringHash,
NULL, utils_stringEquals, NULL);
- }
-
- framework_logIfError(celix_frameworkLogger_globalLogger(), status,
NULL, "Cannot create manifest");
+ celix_status_t status = CELIX_SUCCESS;
+ do {
+ celix_autofree manifest_pt manifestPtr = NULL;
+ manifestPtr = malloc(sizeof(**manifest));
+ if (manifestPtr == NULL) {
+ status = CELIX_ENOMEM;
+ break;
+ }
+ celix_autoptr(celix_properties_t) mainAttributes =
celix_properties_create();
+ if (mainAttributes == NULL) {
+ status = CELIX_ENOMEM;
+ break;
+ }
+ manifestPtr->attributes = hashMap_create(utils_stringHash, NULL,
utils_stringEquals, NULL);
+ if (manifestPtr->attributes == NULL) {
+ status = CELIX_ENOMEM;
+ break;
+ }
+ manifestPtr->mainAttributes = celix_steal_ptr(mainAttributes);
+ *manifest = celix_steal_ptr(manifestPtr);
+ } while(false);
- return status;
+ if (status != CELIX_SUCCESS) {
+ celix_err_pushf("Cannot create manifest: %s", celix_strerror(status));
+ }
+ return status;
}
manifest_pt manifest_clone(manifest_pt manifest) {
- celix_status_t status = CELIX_SUCCESS;
+ celix_status_t status = CELIX_SUCCESS;
- manifest_pt clone = NULL;
- status = manifest_create(&clone);
- if (status == CELIX_SUCCESS) {
- hash_map_iterator_t iter =
hashMapIterator_construct(manifest->attributes);
- while (hashMapIterator_hasNext(&iter)) {
- hash_map_entry_pt entry =
hashMapIterator_nextEntry(&iter);
- char *key = hashMapEntry_getKey(entry);
- celix_properties_t* value =
hashMapEntry_getValue(entry);
- celix_properties_t* cloneValue =
celix_properties_copy(value);
- hashMap_put(clone->attributes, key, cloneValue);
- }
- }
+ celix_auto(manifest_pt) clone = NULL;
+ status = manifest_create(&clone);
+ if (status == CELIX_SUCCESS) {
+ const char* key = NULL;
+ CELIX_PROPERTIES_FOR_EACH(manifest->mainAttributes, key) {
+ celix_properties_set(clone->mainAttributes, key,
celix_properties_get(manifest->mainAttributes, key, NULL));
+ }
+ hash_map_iterator_t iter =
hashMapIterator_construct(manifest->attributes);
+ while (hashMapIterator_hasNext(&iter)) {
+ hash_map_entry_pt entry = hashMapIterator_nextEntry(&iter);
+ celix_autofree char* attrKey =
celix_utils_strdup(hashMapEntry_getKey(entry));
+ if (attrKey == NULL) {
+ return NULL;
+ }
+ celix_properties_t* value = hashMapEntry_getValue(entry);
+ celix_properties_t* cloneValue = celix_properties_copy(value);
+ if (cloneValue == NULL) {
+ return NULL;
+ }
+ hashMap_put(clone->attributes, celix_steal_ptr(attrKey),
cloneValue);
+ }
+ }
- return clone;
+ return celix_steal_ptr(clone);
}
celix_status_t manifest_destroy(manifest_pt manifest) {
- if (manifest != NULL) {
- properties_destroy(manifest->mainAttributes);
- hashMap_destroy(manifest->attributes, true, false);
- manifest->mainAttributes = NULL;
- manifest->attributes = NULL;
- free(manifest);
- manifest = NULL;
- }
- return CELIX_SUCCESS;
+ if (manifest != NULL) {
+ properties_destroy(manifest->mainAttributes);
+ hash_map_iterator_t iter =
hashMapIterator_construct(manifest->attributes);
+ while (hashMapIterator_hasNext(&iter)) {
+ hash_map_entry_pt entry = hashMapIterator_nextEntry(&iter);
+ celix_properties_t* value = hashMapEntry_getValue(entry);
+ celix_properties_destroy(value);
+ }
+ hashMap_destroy(manifest->attributes, true, false);
+ manifest->mainAttributes = NULL;
+ manifest->attributes = NULL;
+ free(manifest);
+ manifest = NULL;
+ }
+ return CELIX_SUCCESS;
}
celix_status_t manifest_createFromFile(const char *filename, manifest_pt
*manifest) {
- celix_status_t status;
+ celix_status_t status;
- status = manifest_create(manifest);
+ celix_auto(manifest_pt) manifestNew = NULL;
+ status = manifest_create(&manifestNew);
- if (status == CELIX_SUCCESS) {
- manifest_read(*manifest, filename);
- }
-
- framework_logIfError(celix_frameworkLogger_globalLogger(), status,
NULL, "Cannot create manifest from file");
-
- return status;
+ status = CELIX_DO_IF(status, manifest_read(manifestNew, filename));
+ if (status == CELIX_SUCCESS) {
+ *manifest = celix_steal_ptr(manifestNew);
+ } else {
+ celix_err_pushf("Cannot create manifest from file: %s",
celix_strerror(status));
+ }
+ return status;
}
+//LCOV_EXCL_START
void manifest_clear(manifest_pt manifest) {
}
+//LCOV_EXCL_STOP
properties_pt manifest_getMainAttributes(manifest_pt manifest) {
- return manifest->mainAttributes;
+ return manifest->mainAttributes;
}
celix_status_t manifest_getEntries(manifest_pt manifest, hash_map_pt *map) {
- *map = manifest->attributes;
- return CELIX_SUCCESS;
+ *map = manifest->attributes;
+ return CELIX_SUCCESS;
}
celix_status_t manifest_read(manifest_pt manifest, const char *filename) {
celix_status_t status = CELIX_SUCCESS;
- FILE *file = fopen ( filename, "r" );
- if (file != NULL) {
- char lbuf[512];
- char name[512];
- bool skipEmptyLines = true;
- char lastline[512];
- memset(lbuf,0,512);
- memset(name,0,512);
- memset(lastline,0,512);
-
- manifest_readAttributes(manifest, manifest->mainAttributes,
file);
-
- while (status==CELIX_SUCCESS && fgets(lbuf, sizeof(lbuf), file)
!= NULL) {
- properties_pt attributes;
- int len = strlen(lbuf);
-
- if (lbuf[--len] != '\n') {
- status = CELIX_FILE_IO_EXCEPTION;
-
framework_logIfError(celix_frameworkLogger_globalLogger(), status, NULL,
"Manifest '%s' line too long", filename);
- break;
- }
- if (len > 0 && lbuf[len - 1] == '\r') {
- --len;
- }
- if (len == 0 && skipEmptyLines) {
- continue;
- }
- skipEmptyLines = false;
-
- if (strlen(name) == 0) {
-
- if ((tolower(lbuf[0]) == 'n') &&
(tolower(lbuf[1]) == 'a') &&
- (tolower(lbuf[2]) == 'm') &&
(tolower(lbuf[3]) == 'e') &&
- (lbuf[4] == ':') && (lbuf[5] == ' ')) {
- name[0] = '\0';
- strncpy(name, lbuf+6, len - 6);
- name[len - 6] = '\0';
- } else {
- status = CELIX_FILE_IO_EXCEPTION;
-
framework_logIfError(celix_frameworkLogger_globalLogger(), status, NULL,
"Manifest '%s' invalid format", filename);
- break;
- }
-
- if (fpeek(file) == ' ') {
- int newlen = len - 6;
- lastline[0] = '\0';
- strncpy(lastline, lbuf+6, len - 6);
- lastline[newlen] = '\0';
- continue;
- }
- } else {
- int newlen = strlen(lastline) + len;
- char buf[512];
- buf[0] = '\0';
- strcpy(buf, lastline);
- strncat(buf, lbuf+1, len - 1);
- buf[newlen] = '\0';
+ celix_autoptr(FILE) file = fopen(filename, "r");
+ if (file != NULL) {
+ status = manifest_readFromStream(manifest, file);
+ } else {
+ status = CELIX_ERROR_MAKE(CELIX_FACILITY_CERRNO,errno);
+ }
- if (fpeek(file) == ' ') {
-// lastline = realloc(lastline,
strlen(buf) + 1);
- lastline[0] = '\0';
- strcpy(lastline, buf);
- continue;
- }
- name[0] = '\0';
- strcpy(name, buf);
- name[strlen(buf)] = '\0';
- }
+ if (status != CELIX_SUCCESS) {
+ celix_err_pushf("Cannot read manifest %s: %s", filename,
celix_strerror(status));
+ }
- attributes = hashMap_get(manifest->attributes, name);
- if (attributes == NULL) {
- attributes = properties_create();
- hashMap_put(manifest->attributes, strdup(name),
attributes);
- }
- manifest_readAttributes(manifest, attributes, file);
+ return status;
+}
- name[0] = '\0';
- skipEmptyLines = true;
- }
- fclose(file);
- } else {
- status = CELIX_FILE_IO_EXCEPTION;
- }
+celix_status_t manifest_readFromStream(manifest_pt manifest, FILE* stream) {
+ char lbuf[512];
+ char *bytes = lbuf;
- if (status != CELIX_SUCCESS) {
- fw_logCode(celix_frameworkLogger_globalLogger(),
CELIX_LOG_LEVEL_ERROR, status, "Cannot read manifest");
+ // get file size
+ if(fseek(stream, 0L, SEEK_END) == -1) {
Review Comment:
nitpick: missing space between `if` and `(`
##########
libs/framework/gtest/src/ManifestTestSuite.cc:
##########
@@ -0,0 +1,307 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include <gtest/gtest.h>
+#include <string>
+#include <stdio.h>
+#if CELIX_UTILS_NO_MEMSTREAM_AVAILABLE
+#include "open_memstream.h"
+#include "fmemopen.h"
+#endif
+
+#include "celix_err.h"
+#include "celix_stdio_cleanup.h"
+#include "manifest.h"
+
+class ManifestTestSuite : public ::testing::Test {
Review Comment:
Nice addition of additional unit test for the manifest parsing :smile:
Also with this PR I think we have reached the 80% overall code coverage
threshold :rocket:
##########
libs/framework/src/manifest.c:
##########
@@ -24,271 +24,291 @@
* \copyright Apache License, Version 2.0
*/
-#include <stdio.h>
+#include <limits.h>
#include <stdlib.h>
#include <string.h>
#include <stdbool.h>
+#include "celix_err.h"
+#include "celix_errno.h"
+#include "celix_stdio_cleanup.h"
+#include "celix_stdlib_cleanup.h"
+#include "celix_utils.h"
#include "manifest.h"
#include "utils.h"
-#include "celix_log.h"
-
-int fpeek(FILE *stream);
-
-static celix_status_t manifest_readAttributes(manifest_pt manifest,
properties_pt properties, FILE *file);
celix_status_t manifest_create(manifest_pt *manifest) {
- celix_status_t status = CELIX_SUCCESS;
-
- *manifest = malloc(sizeof(**manifest));
- if (!*manifest) {
- status = CELIX_ENOMEM;
- } else {
- (*manifest)->mainAttributes = properties_create();
- (*manifest)->attributes = hashMap_create(utils_stringHash,
NULL, utils_stringEquals, NULL);
- }
-
- framework_logIfError(celix_frameworkLogger_globalLogger(), status,
NULL, "Cannot create manifest");
+ celix_status_t status = CELIX_SUCCESS;
+ do {
+ celix_autofree manifest_pt manifestPtr = NULL;
+ manifestPtr = malloc(sizeof(**manifest));
+ if (manifestPtr == NULL) {
+ status = CELIX_ENOMEM;
+ break;
+ }
+ celix_autoptr(celix_properties_t) mainAttributes =
celix_properties_create();
+ if (mainAttributes == NULL) {
+ status = CELIX_ENOMEM;
+ break;
+ }
+ manifestPtr->attributes = hashMap_create(utils_stringHash, NULL,
utils_stringEquals, NULL);
+ if (manifestPtr->attributes == NULL) {
+ status = CELIX_ENOMEM;
+ break;
+ }
+ manifestPtr->mainAttributes = celix_steal_ptr(mainAttributes);
+ *manifest = celix_steal_ptr(manifestPtr);
+ } while(false);
- return status;
+ if (status != CELIX_SUCCESS) {
+ celix_err_pushf("Cannot create manifest: %s", celix_strerror(status));
+ }
+ return status;
}
manifest_pt manifest_clone(manifest_pt manifest) {
- celix_status_t status = CELIX_SUCCESS;
+ celix_status_t status = CELIX_SUCCESS;
- manifest_pt clone = NULL;
- status = manifest_create(&clone);
- if (status == CELIX_SUCCESS) {
- hash_map_iterator_t iter =
hashMapIterator_construct(manifest->attributes);
- while (hashMapIterator_hasNext(&iter)) {
- hash_map_entry_pt entry =
hashMapIterator_nextEntry(&iter);
- char *key = hashMapEntry_getKey(entry);
- celix_properties_t* value =
hashMapEntry_getValue(entry);
- celix_properties_t* cloneValue =
celix_properties_copy(value);
- hashMap_put(clone->attributes, key, cloneValue);
- }
- }
+ celix_auto(manifest_pt) clone = NULL;
+ status = manifest_create(&clone);
+ if (status == CELIX_SUCCESS) {
+ const char* key = NULL;
+ CELIX_PROPERTIES_FOR_EACH(manifest->mainAttributes, key) {
+ celix_properties_set(clone->mainAttributes, key,
celix_properties_get(manifest->mainAttributes, key, NULL));
+ }
+ hash_map_iterator_t iter =
hashMapIterator_construct(manifest->attributes);
+ while (hashMapIterator_hasNext(&iter)) {
+ hash_map_entry_pt entry = hashMapIterator_nextEntry(&iter);
+ celix_autofree char* attrKey =
celix_utils_strdup(hashMapEntry_getKey(entry));
+ if (attrKey == NULL) {
+ return NULL;
+ }
+ celix_properties_t* value = hashMapEntry_getValue(entry);
+ celix_properties_t* cloneValue = celix_properties_copy(value);
+ if (cloneValue == NULL) {
+ return NULL;
+ }
+ hashMap_put(clone->attributes, celix_steal_ptr(attrKey),
cloneValue);
+ }
+ }
- return clone;
+ return celix_steal_ptr(clone);
}
celix_status_t manifest_destroy(manifest_pt manifest) {
- if (manifest != NULL) {
- properties_destroy(manifest->mainAttributes);
- hashMap_destroy(manifest->attributes, true, false);
- manifest->mainAttributes = NULL;
- manifest->attributes = NULL;
- free(manifest);
- manifest = NULL;
- }
- return CELIX_SUCCESS;
+ if (manifest != NULL) {
+ properties_destroy(manifest->mainAttributes);
+ hash_map_iterator_t iter =
hashMapIterator_construct(manifest->attributes);
+ while (hashMapIterator_hasNext(&iter)) {
+ hash_map_entry_pt entry = hashMapIterator_nextEntry(&iter);
+ celix_properties_t* value = hashMapEntry_getValue(entry);
+ celix_properties_destroy(value);
+ }
+ hashMap_destroy(manifest->attributes, true, false);
+ manifest->mainAttributes = NULL;
+ manifest->attributes = NULL;
+ free(manifest);
+ manifest = NULL;
+ }
+ return CELIX_SUCCESS;
}
celix_status_t manifest_createFromFile(const char *filename, manifest_pt
*manifest) {
- celix_status_t status;
+ celix_status_t status;
- status = manifest_create(manifest);
+ celix_auto(manifest_pt) manifestNew = NULL;
+ status = manifest_create(&manifestNew);
- if (status == CELIX_SUCCESS) {
- manifest_read(*manifest, filename);
- }
-
- framework_logIfError(celix_frameworkLogger_globalLogger(), status,
NULL, "Cannot create manifest from file");
-
- return status;
+ status = CELIX_DO_IF(status, manifest_read(manifestNew, filename));
+ if (status == CELIX_SUCCESS) {
+ *manifest = celix_steal_ptr(manifestNew);
+ } else {
+ celix_err_pushf("Cannot create manifest from file: %s",
celix_strerror(status));
+ }
+ return status;
}
+//LCOV_EXCL_START
void manifest_clear(manifest_pt manifest) {
}
+//LCOV_EXCL_STOP
properties_pt manifest_getMainAttributes(manifest_pt manifest) {
- return manifest->mainAttributes;
+ return manifest->mainAttributes;
}
celix_status_t manifest_getEntries(manifest_pt manifest, hash_map_pt *map) {
- *map = manifest->attributes;
- return CELIX_SUCCESS;
+ *map = manifest->attributes;
+ return CELIX_SUCCESS;
}
celix_status_t manifest_read(manifest_pt manifest, const char *filename) {
celix_status_t status = CELIX_SUCCESS;
- FILE *file = fopen ( filename, "r" );
- if (file != NULL) {
- char lbuf[512];
- char name[512];
- bool skipEmptyLines = true;
- char lastline[512];
- memset(lbuf,0,512);
- memset(name,0,512);
- memset(lastline,0,512);
-
- manifest_readAttributes(manifest, manifest->mainAttributes,
file);
-
- while (status==CELIX_SUCCESS && fgets(lbuf, sizeof(lbuf), file)
!= NULL) {
- properties_pt attributes;
- int len = strlen(lbuf);
-
- if (lbuf[--len] != '\n') {
- status = CELIX_FILE_IO_EXCEPTION;
-
framework_logIfError(celix_frameworkLogger_globalLogger(), status, NULL,
"Manifest '%s' line too long", filename);
- break;
- }
- if (len > 0 && lbuf[len - 1] == '\r') {
- --len;
- }
- if (len == 0 && skipEmptyLines) {
- continue;
- }
- skipEmptyLines = false;
-
- if (strlen(name) == 0) {
-
- if ((tolower(lbuf[0]) == 'n') &&
(tolower(lbuf[1]) == 'a') &&
- (tolower(lbuf[2]) == 'm') &&
(tolower(lbuf[3]) == 'e') &&
- (lbuf[4] == ':') && (lbuf[5] == ' ')) {
- name[0] = '\0';
- strncpy(name, lbuf+6, len - 6);
- name[len - 6] = '\0';
- } else {
- status = CELIX_FILE_IO_EXCEPTION;
-
framework_logIfError(celix_frameworkLogger_globalLogger(), status, NULL,
"Manifest '%s' invalid format", filename);
- break;
- }
-
- if (fpeek(file) == ' ') {
- int newlen = len - 6;
- lastline[0] = '\0';
- strncpy(lastline, lbuf+6, len - 6);
- lastline[newlen] = '\0';
- continue;
- }
- } else {
- int newlen = strlen(lastline) + len;
- char buf[512];
- buf[0] = '\0';
- strcpy(buf, lastline);
- strncat(buf, lbuf+1, len - 1);
- buf[newlen] = '\0';
+ celix_autoptr(FILE) file = fopen(filename, "r");
+ if (file != NULL) {
+ status = manifest_readFromStream(manifest, file);
+ } else {
+ status = CELIX_ERROR_MAKE(CELIX_FACILITY_CERRNO,errno);
+ }
- if (fpeek(file) == ' ') {
-// lastline = realloc(lastline,
strlen(buf) + 1);
- lastline[0] = '\0';
- strcpy(lastline, buf);
- continue;
- }
- name[0] = '\0';
- strcpy(name, buf);
- name[strlen(buf)] = '\0';
- }
+ if (status != CELIX_SUCCESS) {
+ celix_err_pushf("Cannot read manifest %s: %s", filename,
celix_strerror(status));
+ }
- attributes = hashMap_get(manifest->attributes, name);
- if (attributes == NULL) {
- attributes = properties_create();
- hashMap_put(manifest->attributes, strdup(name),
attributes);
- }
- manifest_readAttributes(manifest, attributes, file);
+ return status;
+}
- name[0] = '\0';
- skipEmptyLines = true;
- }
- fclose(file);
- } else {
- status = CELIX_FILE_IO_EXCEPTION;
- }
+celix_status_t manifest_readFromStream(manifest_pt manifest, FILE* stream) {
+ char lbuf[512];
+ char *bytes = lbuf;
- if (status != CELIX_SUCCESS) {
- fw_logCode(celix_frameworkLogger_globalLogger(),
CELIX_LOG_LEVEL_ERROR, status, "Cannot read manifest");
+ // get file size
+ if(fseek(stream, 0L, SEEK_END) == -1) {
+ return CELIX_ERROR_MAKE(CELIX_FACILITY_CERRNO,errno);
+ }
+ long int size = ftell(stream);
+ if (size < 0) {
+ return CELIX_ERROR_MAKE(CELIX_FACILITY_CERRNO,errno);
+ } else if (size >= INT_MAX) {
+ celix_err_pushf("Manifest error: file too large - %ld", size);
+ return CELIX_BUNDLE_EXCEPTION;
+ }
+ rewind(stream);
+
+ celix_autofree char* largeBuf = NULL;
+ if (size+1 > sizeof(lbuf)) {
Review Comment:
nitpick: you have `lbuf` and `largeBuf`, I assume that `lbuf` means local
buf. But in this case for clearity I would prefer something like `stackBuf` and
`headBuf`
##########
libs/utils/include/celix_err.h:
##########
@@ -82,6 +83,17 @@ CELIX_UTILS_EXPORT void celix_err_resetErrors();
*/
CELIX_UTILS_EXPORT void celix_err_printErrors(FILE* stream, const char*
prefix, const char* postfix);
+/**
+ * @brief Dump error messages from the current thread to the provided buffer.
+ * @param[in] buf The buffer to dump the error messages to.
Review Comment:
nitpick: the buf is a inout parameter (after the call the buf is used)
--
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
