PengZheng commented on code in PR #537:
URL: https://github.com/apache/celix/pull/537#discussion_r1184494919
##########
libs/error_injector/celix_bundle_ctx/src/celix_bundle_context_ei.cc:
##########
@@ -85,4 +85,14 @@ long
__wrap_celix_bundleContext_registerServiceAsync(celix_bundle_context_t *__c
return __real_celix_bundleContext_registerServiceAsync(__ctx,
__serviceName, __service, __properties);
}
+long
__real_celix_bundleContext_registerServiceFactoryAsync(celix_bundle_context_t
*__ctx, const char *__serviceName, service_factory_pt __factory,
celix_properties_t *__properties);
+CELIX_EI_DEFINE(celix_bundleContext_registerServiceFactoryAsync, long)
+long
__wrap_celix_bundleContext_registerServiceFactoryAsync(celix_bundle_context_t
*__ctx, const char *__serviceName, service_factory_pt __factory,
celix_properties_t *__properties) {
+ celix_properties_t __attribute__((cleanup(celix_properties_cleanup)))
*props = celix_properties_copy(__properties);
+ celix_properties_destroy(__properties);
+ CELIX_EI_IMPL(celix_bundleContext_registerServiceFactoryAsync);
+ __properties = celix_properties_copy(props);
Review Comment:
Interesting trick.
##########
libs/error_injector/CMakeLists.txt:
##########
@@ -39,6 +39,9 @@ add_subdirectory(sys_shm)
add_subdirectory(socket)
add_subdirectory(pthread)
add_subdirectory(celix_log_helper)
+add_subdirectory(celix_bundle)
+add_subdirectory(celix_version)
+add_subdirectory(dfi)
Review Comment:
dfi is an optional component, thus we need to check `CELIX_DFI` before
`add_subdirectory`.
##########
bundles/remote_services/rsa_rpc_json/src/rsa_json_rpc_proxy_impl.c:
##########
@@ -294,19 +298,18 @@ static celix_status_t
rsaJsonRpcProxy_create(rsa_json_rpc_proxy_factory_t *proxy
celix_logHelper_error(proxyFactory->logHelper, "Proxy: Error getting
provider service version.");
goto err_getting_svc_ver;
}
- version_pt providerVersion;
- status =version_createVersionFromString(providerVerStr,&providerVersion);
- if (status != CELIX_SUCCESS) {
+ celix_version_t *providerVersion =
celix_version_createVersionFromString(providerVerStr);
+ if (providerVersion == NULL) {
celix_logHelper_error(proxyFactory->logHelper, "Proxy: Error
converting service version type. %d.", status);
+ status = CELIX_ENOMEM;
goto err_creating_provider_ver;
}
- version_pt consumerVersion = NULL;
+ celix_version_t *consumerVersion = NULL;
bool isCompatible = false;
dynInterface_getVersion(intfType,&consumerVersion);
- version_isCompatible(consumerVersion, providerVersion,&isCompatible);
+ isCompatible = celix_version_isCompatible(consumerVersion,
providerVersion);
if(!isCompatible){
- char* consumerVerStr = NULL;
- version_toString(consumerVersion,&consumerVerStr);
+ char* consumerVerStr = celix_version_toString(consumerVersion);
Review Comment:
This involves dynamic allocation (thus possibility of failure). We can get
the same information by
`celix_version_getMajor`/`celix_version_getMinor`/`celix_version_getMicro`.
##########
bundles/remote_services/remote_service_admin_shm_v2/rsa_shm/src/rsa_shm_server.c:
##########
@@ -193,51 +200,48 @@ static void rsaShmServer_msgHandlingWork(void *data) {
char *src = reply.iov_base;
size_t srcSize = reply.iov_len;
+ int waitRet = 0;
+ pthread_mutex_lock(&msgCtrl->lock);
while (true) {
- ssize_t destSize = workData->msgBodyTotalSize;
+ if (msgCtrl->msgState == REQ_CANCELLED || waitRet != 0) {
+ celix_logHelper_error(server->loghelper, "RsaShmServer: Client
cancelled the request, or timeout. %d.", waitRet);
Review Comment:
Performing IO while holding a lock is always sub-optimal.
##########
bundles/remote_services/remote_service_admin_shm_v2/rsa_shm/src/rsa_shm_impl.c:
##########
@@ -269,10 +293,13 @@ static bool rsaShm_isConfigTypeMatched(celix_properties_t
*properties) {
token = strtok_r(ecCopy, delimiter, &savePtr);
while (token != NULL) {
- if (strncmp(utils_stringTrim(token), RSA_SHM_CONFIGURATION_TYPE,
1024) == 0) {
+ char *configType = celix_utils_trim(token);
+ if (configType != NULL && strncmp(configType,
RSA_SHM_CONFIGURATION_TYPE, 1024) == 0) {
matched = true;
+ free(configType);
Review Comment:
This seems cumbersome.
##########
bundles/remote_services/remote_service_admin_shm_v2/shm_pool/src/shm_pool.c:
##########
@@ -181,8 +185,8 @@ static void *shmPool_heartbeatThread(void *data){
celixThreadMutex_lock(&pool->mutex);
int waitRet = 0;
while (pool->heartbeatThreadActive && waitRet != ETIMEDOUT) {
+ // pthread_cond_timedwait shall not return an error code of [EINTR]
Review Comment:
The following link should be added, since `man pthread_cond_timedwait` on
Ubuntu 22.04 says the reverse.
https://man7.org/linux/man-pages/man3/pthread_cond_timedwait.3p.html
These functions shall not return an error code of [EINTR].
##########
bundles/remote_services/rsa_rpc_json/src/rsa_json_rpc_activator.c:
##########
@@ -63,7 +66,7 @@ static celix_status_t
rsaJsonRpc_start(rsa_json_rpc_activator_t* activator, celi
activator->rpcSvcId =
celix_bundleContext_registerServiceWithOptionsAsync(ctx, &opts);
if (activator->rpcSvcId < 0) {
celix_logHelper_error(activator->logHelper, "Error registering json
rpc service.");
- status = CELIX_SERVICE_EXCEPTION;
+ status = CELIX_BUNDLE_EXCEPTION;
Review Comment:
The above `celix_properties_create` should also be checked for failure.
But this can be left for a future PR. No hurry.
##########
libs/error_injector/stdio/src/stdio_ei.cc:
##########
@@ -49,4 +49,14 @@ int __wrap_remove (const char *__filename) {
errno = 0;
return __real_remove(__filename);
}
+
+FILE *__real_open_memstream (char **__bufloc, size_t *__sizeloc);
+CELIX_EI_DEFINE(open_memstream, FILE *)
+FILE *__wrap_open_memstream (char **__bufloc, size_t *__sizeloc) {
+ errno = EMFILE;
Review Comment:
It does not open any file, thus `EMFILE` is not a good choice.
##########
bundles/remote_services/remote_service_admin_shm_v2/rsa_shm/src/rsa_shm_client.c:
##########
@@ -179,8 +181,8 @@ void rsaShmClientManager_destory(rsa_shm_client_manager_t
*clientManager) {
size_t listSize = celix_arrayList_size(clientManager->exceptionMsgList);
for (int i = 0; i < listSize; ++i) {
rsa_shm_exception_msg_t *exceptionMsg =
celix_arrayList_get(clientManager->exceptionMsgList, i);
- shmPool_free(clientManager->shmPool, exceptionMsg->msgBuffer);
- rsaShmClientManager_destroyMsgControl(clientManager,
exceptionMsg->msgCtrl);
+ //Here，we should not free 'msgBuffer' and 'msgCtrl' by shmPool_free,
because rsa_shm_server maybe using them, and tlsf_free will modify freed memory.
+ //The shared memory of 'msgBuffer' and 'msgCtrl' will be freed
automatically when nobody is using it.
Review Comment:
I consider it a design flaw, i.e., lack of synchronization between server
and client.
Of course, if `pthread_cond_destroy` and `pthread_mutex_destroy` does not
release any resource like kernel objects, then no real issue should arise.
##########
bundles/remote_services/remote_service_admin_shm_v2/rsa_shm/src/rsa_shm_impl.c:
##########
@@ -269,10 +293,13 @@ static bool rsaShm_isConfigTypeMatched(celix_properties_t
*properties) {
token = strtok_r(ecCopy, delimiter, &savePtr);
while (token != NULL) {
- if (strncmp(utils_stringTrim(token), RSA_SHM_CONFIGURATION_TYPE,
1024) == 0) {
+ char *configType = celix_utils_trim(token);
+ if (configType != NULL && strncmp(configType,
RSA_SHM_CONFIGURATION_TYPE, 1024) == 0) {
matched = true;
+ free(configType);
Review Comment:
I suggest we repromote `utils_stringTrim` into the Celix API (adding celix_
prefix), and recover its proper uses all over the code base.
WDYT? @xuzhenbao @pnoltes
##########
bundles/remote_services/remote_service_admin_shm_v2/rsa_shm/src/rsa_shm_import_registration.c:
##########
@@ -71,14 +76,17 @@ celix_status_t
importRegistration_create(celix_bundle_context_t *context,
char *token, *savePtr;
token = strtok_r(icCopy, delimiter, &savePtr);
while (token != NULL) {
- if (strncmp(utils_stringTrim(token), RSA_RPC_TYPE_PREFIX,
sizeof(RSA_RPC_TYPE_PREFIX) - 1) == 0) {
- rsaRpcType = token;
+ rsaRpcType = celix_utils_trim(token);
Review Comment:
I think `utils_stringTrim` is a better choice for this case.
Why did we deprecate it? @pnoltes
##########
bundles/remote_services/remote_service_admin_shm_v2/rsa_shm/src/rsa_shm_import_registration.c:
##########
@@ -48,12 +47,17 @@ celix_status_t
importRegistration_create(celix_bundle_context_t *context,
return CELIX_ILLEGAL_ARGUMENT;
}
import_registration_t *import = (import_registration_t *)calloc(1,
sizeof(*import));
- assert(import != NULL);
+ if (import == NULL) {
+ return CELIX_ENOMEM;
+ }
import->context = context;
import->logHelper = logHelper;
import->endpointDesc = endpointDescription_clone(endpointDesc);
- assert(import->endpointDesc != NULL);
+ if (import->endpointDesc == NULL) {
Review Comment:
This error is not handled in `rsa_json_rpc`.
##########
bundles/remote_services/remote_service_admin_shm_v2/shm_pool/src/shm_pool.c:
##########
@@ -72,7 +75,7 @@ celix_status_t shmPool_create(size_t size, shm_pool_t **pool)
{
}
shmPool->shmStartAddr = shmat(shmPool->shmId, NULL, 0);
if (shmPool->shmStartAddr == NULL) {
- fprintf(stderr,"Shm pool: Error sttaching shm attach, %d.\n",errno);
+ fprintf(stderr,"Shm pool: Error attaching shm, %d.\n",errno);
Review Comment:
A separate issue is created for this: #539
--
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
