Re: [PR] Feature/674 use properties type in filter (celix)

[via GitHub]

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


##########
libs/framework/src/bundle_context.c:
##########
@@ -389,15 +392,23 @@ static long 
celix_bundleContext_registerServiceWithOptionsInternal(bundle_contex
     }
 
     //set properties
-    celix_properties_t *props = opts->properties;
+    celix_autoptr(celix_properties_t) props = opts->properties;
     if (props == NULL) {
         props = celix_properties_create();
     }
+
     if (opts->serviceVersion != NULL && strncmp("", opts->serviceVersion, 1) 
!= 0) {
-        celix_properties_set(props, CELIX_FRAMEWORK_SERVICE_VERSION, 
opts->serviceVersion);
+        celix_version_t* version = 
celix_version_createVersionFromString(opts->serviceVersion);
+        if (!version) {
+            celix_framework_logTssErrors(ctx->framework->logger, 
CELIX_LOG_LEVEL_ERROR);
+            fw_log(
+                ctx->framework->logger, CELIX_LOG_LEVEL_ERROR, "Cannot parse 
service version %s", opts->serviceVersion);
+            return -1;
+        }
+        celix_properties_setVersionWithoutCopy(props, 
CELIX_FRAMEWORK_SERVICE_VERSION, version);

Review Comment:
   Error handling is needed.



##########
libs/framework/src/dm_component_impl.c:
##########
@@ -445,29 +445,42 @@ celix_status_t 
component_addInterface(celix_dm_component_t *component, const cha
     return celix_dmComponent_addInterface(component, serviceName, 
serviceVersion, service, properties);
 }
 
-celix_status_t celix_dmComponent_addInterface(celix_dm_component_t *component, 
const char* serviceName, const char* serviceVersion, const void* service, 
celix_properties_t* properties) {
+celix_status_t celix_dmComponent_addInterface(celix_dm_component_t* component,
+                                              const char* serviceName,
+                                              const char* serviceVersion,
+                                              const void* service,
+                                              celix_properties_t* properties) {
     if (serviceName == NULL || celix_utils_stringEquals(serviceName, "")) {
-        celix_bundleContext_log(component->context, CELIX_LOG_LEVEL_ERROR, 
"Cannot add interface with a NULL or empty serviceName");
+        celix_bundleContext_log(
+            component->context, CELIX_LOG_LEVEL_ERROR, "Cannot add interface 
with a NULL or empty serviceName");
         return CELIX_ILLEGAL_ARGUMENT;
     }
 
-    dm_interface_t *interface = calloc(1, sizeof(*interface));
-    char *name = celix_utils_strdup(serviceName);
+    dm_interface_t* interface = calloc(1, sizeof(*interface));
+    char* name = celix_utils_strdup(serviceName);
 
     if (properties == NULL) {
         properties = celix_properties_create();
     }
 
-    if ((properties_get(properties, CELIX_FRAMEWORK_SERVICE_VERSION) == NULL) 
&& (serviceVersion != NULL)) {
-        celix_properties_set(properties, CELIX_FRAMEWORK_SERVICE_VERSION, 
serviceVersion);
+    if (serviceVersion != NULL) {
+        celix_version_t* version = 
celix_version_createVersionFromString(serviceVersion);
+        if (!version) {
+            celix_bundleContext_log(
+                component->context, CELIX_LOG_LEVEL_ERROR, "Cannot add 
interface with an invalid serviceVersion");
+            celix_properties_destroy(properties);

Review Comment:
   `interface` and `name` are leaked.



-- 
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