You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@celix.apache.org by GitBox <gi...@apache.org> on 2022/01/19 14:04:00 UTC

[GitHub] [celix] PengZheng opened a new pull request #392: Tracker improvements preliminary

PengZheng opened a new pull request #392:
URL: https://github.com/apache/celix/pull/392


   As mentioned in https://github.com/apache/celix/pull/376#issuecomment-965961677, I'm trying to improve the performance of ServiceTracker.  Before the real work done, I must familiarize myself of the code base. Along the way, I do some code cleanup and bug fixes.  Some worth noting:
   
   - buggy default array_list equality implementation. To avoid this kind of bugs forever, I add compile time assert (static assert in modern C++).
   - wrong bundle ID in service tracker callbacks.
   - flawed reference counting implementation, as discussed in #390 


-- 
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-unsubscribe@celix.apache.org

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

[GitHub] [celix] pnoltes merged pull request #392: Tracker improvements preliminary

Posted by GitBox <gi...@apache.org>.
pnoltes merged pull request #392:
URL: https://github.com/apache/celix/pull/392


   


-- 
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-unsubscribe@celix.apache.org

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

[GitHub] [celix] PengZheng commented on a change in pull request #392: Tracker improvements preliminary

Posted by GitBox <gi...@apache.org>.
PengZheng commented on a change in pull request #392:
URL: https://github.com/apache/celix/pull/392#discussion_r790626170



##########
File path: libs/framework/src/bundle_context.c
##########
@@ -76,7 +76,7 @@ celix_status_t bundleContext_create(framework_pt framework, celix_framework_logg
 
         }
 	}
-
+    //FIXME: context == NULL?

Review comment:
       Thanks for making the policy clear. I accept it and remove the FIXME.
   
   Please note that Linux overcommit can be turned off and that malloc/calloc can fail for other reasons, .e.g. ulimit.
   
   One particular failure worth noting is that calloc return NULL when integer overflow is detected (nmemb * size):
   https://github.com/lattera/glibc/blob/master/malloc/malloc.c#L3361-L3370
   
   If one of `nmemb` and `size` is from external source (like user input), it's kind of Denial of Service vulnerability.




-- 
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-unsubscribe@celix.apache.org

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

[GitHub] [celix] pnoltes commented on a change in pull request #392: Tracker improvements preliminary

Posted by GitBox <gi...@apache.org>.
pnoltes commented on a change in pull request #392:
URL: https://github.com/apache/celix/pull/392#discussion_r795030461



##########
File path: libs/framework/src/bundle_context.c
##########
@@ -76,7 +76,7 @@ celix_status_t bundleContext_create(framework_pt framework, celix_framework_logg
 
         }
 	}
-
+    //FIXME: context == NULL?

Review comment:
       Thanks for the detailed answer. I am not familiar with the link-time test double approach; Its sound very interesting. 
   
   But to be honest, I am not sure if we need to pursue this for Celix. In my professional I can rely on the Linux oom killer.
   But on the other hand testing mallocs, calloc, pthread, etc on a framework where functionality can be dynamically extended would be helpful. 
   




-- 
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-unsubscribe@celix.apache.org

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

[GitHub] [celix] pnoltes commented on a change in pull request #392: Tracker improvements preliminary

Posted by GitBox <gi...@apache.org>.
pnoltes commented on a change in pull request #392:
URL: https://github.com/apache/celix/pull/392#discussion_r790317824



##########
File path: CMakeLists.txt
##########
@@ -44,7 +44,7 @@ endif ()
 set(ENABLE_MORE_WARNINGS OFF)
 
 # Set C specific flags
-set(CMAKE_C_FLAGS "-D_GNU_SOURCE -std=gnu99 -fPIC ${CMAKE_C_FLAGS}")
+set(CMAKE_C_FLAGS "-D_GNU_SOURCE -std=gnu11 -fPIC ${CMAKE_C_FLAGS}")

Review comment:
       Note: that this will update the C requirements for Celix from gnu99 to gnu11. 
   
   I have no issue with this, but maybe it good to get some broader feedback on this.
   @rlenferink , @rbulter , @ErjanAltena and @stegemr Do any of you have any issues updating the Celix C standard from gnu99 to gnu11? . Of I we have no issue, can we also introduce the use of `#pragma once` for C code?




-- 
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-unsubscribe@celix.apache.org

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

[GitHub] [celix] PengZheng commented on a change in pull request #392: Tracker improvements preliminary

Posted by GitBox <gi...@apache.org>.
PengZheng commented on a change in pull request #392:
URL: https://github.com/apache/celix/pull/392#discussion_r791780927



##########
File path: libs/framework/src/bundle_context.c
##########
@@ -76,7 +76,7 @@ celix_status_t bundleContext_create(framework_pt framework, celix_framework_logg
 
         }
 	}
-
+    //FIXME: context == NULL?

Review comment:
       I guess you are talking about injecting errors into libc or any 3rd party library function call when doing unit test, right? 
   The standard mocking method in C++  is to provide mock implementation of an abstract interface. The C equivalent is using function pointers:
   
   So instead of 
   ```C
   int FormatOutput(const char *, ...);
   ```
   we put the following in the header file:
   ```C
   extern int (*FormatOutput)(const char *, ...);
   ```
   
   Though both of them support runtime-bound test doubles very well, careless use can lead to lots of unnecessary abstract classes/function pointers, which could harm both performance and code readability considerably.  In this particular case (libc/3rd party C libraries), these runtime test doubles are pretty awkward to use,  i.e. we have to re-exposing functions as function pointers instead of using header file directly.
   
   Instead, I use link-time substitution. It does not require any wired wrappers in production codes. We still need wrappers, but they only appear in testing code. It permits testing corner cases which is almost impossible to cover in system test like the following:
   
   ```C
   TEST(AlarmUploader, HeartbeatTimeoutWhenSending)
   {
           NETRET_HEADER header;
   	expectRecv = true;
           //enable link-time test doubles for clock_gettime and gettimeofday
           //if disabled, they will call the libc's implementation
   	EnableClockMock(true);
   	LONGS_EQUAL(EDA_OK, AlarmUploaderInit(conn, module, &attr));
   	LONGS_EQUAL(EDA_OK, AlarmUploaderConnect(conn, NULL, NULL));
   	//enable IO mock to substitute write and writev, which libuv uses for socket IO
           EnableIoMock(true);
   	//limit each write/writev invocation can send out at most 1 byte
   	//so that the transportation of tiny heartbeat packet (< 100 bytes) cannot be completed in one uv_run call
   	SetIoMockMaxBytesPerWrite(1);
           // let 5.001 seconds pass in our fake clock to trigger 5 seconds timeout
   	ClockAdvance(5001); 
           // actually trigger libuv timer with heartbeat pending
   	uv_run(loop, UV_RUN_NOWAIT);
           // disable IO mock, so that libuv send out packet at normal speed
   	EnableIoMock(false);
   	ClockAdvance(5000);
   	ExpectNetRetHeader(&header, NETRET_QUALIFIED);
   	mock().expectOneCall("message_received").withUnsignedIntParameter("status", NETRET_EXCHANGE);
   	mock().expectOneCall("message_received").withUnsignedIntParameter("status", NETRET_EXCHANGE);
   	uv_run(loop, UV_RUN_NOWAIT);
   	ObjectPolyFinish((Object)conn);
   	fd = -1;
           EnableIoMock(false);
   }
   
   ```
   
   Triggering timeout when sending a tiny packet is very difficult (if not possible) in reality, even with the help of expensive network emulation hardware. But with link-time test double, it's easy to inject error into any C library call. Also the test case is extremely fast to execute, since we use fake clock to avoid waiting 10 seconds. High line/branch coverage (like sqlite) is not difficult to achieve.
   
   However, such powerful testing technique is not without its price:
   
   * 1000 LOC `AlarmUploader` takes 3300 LOC to test.
   * Such tests are not written against API, they are completely whitebox.
   * Bad readability as a consequence. API test case can be used as demo while the above test case cannot.
   
   We have to take extra care of the interplay between malloc/calloc/pthread test double and Google Address Sanitizer so that they can work together. I'll prepare a gist for malloc/calloc if Celix wants to take this whitebox way of testing. @pnoltes 




-- 
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-unsubscribe@celix.apache.org

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

[GitHub] [celix] pnoltes commented on a change in pull request #392: Tracker improvements preliminary

Posted by GitBox <gi...@apache.org>.
pnoltes commented on a change in pull request #392:
URL: https://github.com/apache/celix/pull/392#discussion_r790316756



##########
File path: libs/framework/src/service_tracker.c
##########
@@ -94,11 +94,9 @@ celix_status_t serviceTracker_create(bundle_context_pt context, const char * ser
 	if (service == NULL || *tracker != NULL) {
 		status = CELIX_ILLEGAL_ARGUMENT;
 	} else {
-		if (status == CELIX_SUCCESS) {
-			char filter[512];
-			snprintf(filter, sizeof(filter), "(%s=%s)", OSGI_FRAMEWORK_OBJECTCLASS, service);
-            serviceTracker_createWithFilter(context, filter, customizer, tracker);
-		}
+        char filter[512];
+        snprintf(filter, sizeof(filter), "(%s=%s)", OSGI_FRAMEWORK_OBJECTCLASS, service);

Review comment:
       While update this, maybe also use `asprintf` to prevent the situation that 512 char are not enough for the filter. 

##########
File path: libs/framework/src/service_tracker.c
##########
@@ -354,31 +337,20 @@ static void serviceTracker_serviceChanged(void *handle, celix_service_event_t *e
     }
     celixThreadMutex_unlock(&tracker->closeSync.mutex);
 
-    if (!closing) {
-        switch (event->type) {
-            case OSGI_FRAMEWORK_SERVICE_EVENT_REGISTERED:
-                serviceTracker_track(tracker, event->reference, event);
-                break;
-            case OSGI_FRAMEWORK_SERVICE_EVENT_MODIFIED:
+    switch (event->type) {
+        case OSGI_FRAMEWORK_SERVICE_EVENT_REGISTERED:
+        case OSGI_FRAMEWORK_SERVICE_EVENT_MODIFIED:
+            if(!closing)

Review comment:
       Please add `{` `}` for the if statement

##########
File path: CMakeLists.txt
##########
@@ -44,7 +44,7 @@ endif ()
 set(ENABLE_MORE_WARNINGS OFF)
 
 # Set C specific flags
-set(CMAKE_C_FLAGS "-D_GNU_SOURCE -std=gnu99 -fPIC ${CMAKE_C_FLAGS}")
+set(CMAKE_C_FLAGS "-D_GNU_SOURCE -std=gnu11 -fPIC ${CMAKE_C_FLAGS}")

Review comment:
       Note: that this will update the C requirements for Celix from gnu99 to gnu11. 
   
   I have no issue with this, but maybe it good to get some broader feedback on this.
   @rlenferink , @rbulter , @ErjanAltena and @stegemr Do any of you have any uses updating the Celix C standard from gnu99 to gnu11? . Of I we have no issue, can we also introduce the use of `#pragma once` for C code?

##########
File path: libs/framework/src/bundle_context.c
##########
@@ -76,7 +76,7 @@ celix_status_t bundleContext_create(framework_pt framework, celix_framework_logg
 
         }
 	}
-
+    //FIXME: context == NULL?

Review comment:
       In earlier Celix code we tested whether malloc (and calloc) returns a not NULL value. 
   
   In recent code we opted not to check this anymore. For a few reasons:
    - malloc can and will return a (virtual) pointer even if there not enough memory. 
    - Testing of for the "out of memory" iflelse branch is not really doable, resulting in lower test coverage and untested code
    -  For a modern OS the OOM killer will stop the process, in that case there is no need to handle OOM situations.    

##########
File path: libs/utils/include/celix_build_assert.h
##########
@@ -0,0 +1,26 @@
+/*
+ * 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.
+ */
+
+#ifndef CELIX_CELIX_BUILD_ASSERT_H
+#define CELIX_CELIX_BUILD_ASSERT_H
+
+#define BUILD_ASSERT(cond) \

Review comment:
       Yeah for newer code I always prefer a `CELIX_` / `celix_` prefix

##########
File path: libs/utils/include/celix_ref.h
##########
@@ -0,0 +1,54 @@
+/*
+ * 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.
+ */
+
+#ifndef CELIX_CELIX_REF_H
+#define CELIX_CELIX_REF_H
+
+#include <stdatomic.h>
+#include <limits.h>
+#include <assert.h>
+
+struct celix_ref {
+    atomic_int count;
+};
+
+static inline void celix_ref_init(struct celix_ref *ref) {

Review comment:
       I would prefer some doxygen documentation explaining the functions from a users perspective




-- 
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-unsubscribe@celix.apache.org

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

[GitHub] [celix] PengZheng commented on a change in pull request #392: Tracker improvements preliminary

Posted by GitBox <gi...@apache.org>.
PengZheng commented on a change in pull request #392:
URL: https://github.com/apache/celix/pull/392#discussion_r790589661



##########
File path: libs/utils/include/celix_ref.h
##########
@@ -0,0 +1,54 @@
+/*
+ * 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.
+ */
+
+#ifndef CELIX_CELIX_REF_H
+#define CELIX_CELIX_REF_H
+
+#include <stdatomic.h>
+#include <limits.h>
+#include <assert.h>
+
+struct celix_ref {
+    atomic_int count;
+};
+
+static inline void celix_ref_init(struct celix_ref *ref) {

Review comment:
       CELIX_ and Doxygen added.




-- 
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-unsubscribe@celix.apache.org

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

[GitHub] [celix] PengZheng commented on a change in pull request #392: Tracker improvements preliminary

Posted by GitBox <gi...@apache.org>.
PengZheng commented on a change in pull request #392:
URL: https://github.com/apache/celix/pull/392#discussion_r790588776



##########
File path: CMakeLists.txt
##########
@@ -44,7 +44,7 @@ endif ()
 set(ENABLE_MORE_WARNINGS OFF)
 
 # Set C specific flags
-set(CMAKE_C_FLAGS "-D_GNU_SOURCE -std=gnu99 -fPIC ${CMAKE_C_FLAGS}")
+set(CMAKE_C_FLAGS "-D_GNU_SOURCE -std=gnu11 -fPIC ${CMAKE_C_FLAGS}")

Review comment:
       I'd better follow pubsub_admin's usage of gcc built-in atomic operations  to avoid introducing C11.




-- 
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-unsubscribe@celix.apache.org

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

[GitHub] [celix] PengZheng commented on a change in pull request #392: Tracker improvements preliminary

Posted by GitBox <gi...@apache.org>.
PengZheng commented on a change in pull request #392:
URL: https://github.com/apache/celix/pull/392#discussion_r790626170



##########
File path: libs/framework/src/bundle_context.c
##########
@@ -76,7 +76,7 @@ celix_status_t bundleContext_create(framework_pt framework, celix_framework_logg
 
         }
 	}
-
+    //FIXME: context == NULL?

Review comment:
       Thanks for making the policy clear. I accept it and remove the FIXME.
   
   Please note that Linux overcommit can be turned off and that malloc/calloc can fail for other reasons, .e.g. ulimit.
   
   One failure worth noting is that calloc return NULL when integer overflow is detected (nmemb * size):
   https://github.com/lattera/glibc/blob/master/malloc/malloc.c#L3361-L3370
   
   If one of `nmemb` and `size` is from external source (like user input), it's kind of Denial of Service vulnerability.




-- 
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-unsubscribe@celix.apache.org

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

[GitHub] [celix] PengZheng commented on a change in pull request #392: Tracker improvements preliminary

Posted by GitBox <gi...@apache.org>.
PengZheng commented on a change in pull request #392:
URL: https://github.com/apache/celix/pull/392#discussion_r791780927



##########
File path: libs/framework/src/bundle_context.c
##########
@@ -76,7 +76,7 @@ celix_status_t bundleContext_create(framework_pt framework, celix_framework_logg
 
         }
 	}
-
+    //FIXME: context == NULL?

Review comment:
       I guess you are talking about injecting errors into libc or any 3rd party library function call when doing unit test, right? 
   The standard mocking method in C++  is to provide mock implementation of an abstract interface. The C equivalent is using function pointers:
   
   So instead of 
   ```C
   int FormatOutput(const char *, ...);
   ```
   we put the following in the header file:
   ```C
   extern int (*FormatOutput)(const char *, ...);
   ```
   
   Though both of them support runtime-bound test doubles very well, careless use can lead to lots of unnecessary abstract classes/function pointers, which could harm both performance and code readability considerably.  In this particular case (libc/3rd party C libraries), these runtime test doubles are pretty awkward to use,  i.e. we have to re-exposing functions as function pointers instead of using header file directly.
   
   Instead, I use link-time substitution. It does not require any wired wrappers in production codes. We still need wrappers, but they only appear in testing code. It permits testing corner cases which is almost impossible to cover in system test like the following:
   
   ```C
   TEST(AlarmUploader, HeartbeatTimeoutWhenSending)
   {
           NETRET_HEADER header;
   	expectRecv = true;
           //enable link-time test doubles for clock_gettime and gettimeofday
           //if disabled, they will call the libc's implementation
   	EnableClockMock(true);
   	LONGS_EQUAL(EDA_OK, AlarmUploaderInit(conn, module, &attr));
   	LONGS_EQUAL(EDA_OK, AlarmUploaderConnect(conn, NULL, NULL));
   	//enable IO mock to substitute write and writev, which libuv uses for socket IO
           EnableIoMock(true);
   	//limit each write/writev invocation can send out at most 1 byte
   	//so that the transportation of tiny heartbeat packet (< 100 bytes) cannot be completed in one uv_run call
   	SetIoMockMaxBytesPerWrite(1);
           // let 5.001 seconds pass in our fake clock to trigger 5 seconds timeout
   	ClockAdvance(5001); 
           // actually trigger libuv timer with heartbeat pending
   	uv_run(loop, UV_RUN_NOWAIT);
           // disable IO mock, so that libuv send out packet at normal speed
   	EnableIoMock(false);
   	ClockAdvance(5000);
   	ExpectNetRetHeader(&header, NETRET_QUALIFIED);
   	mock().expectOneCall("message_received").withUnsignedIntParameter("status", NETRET_EXCHANGE);
   	mock().expectOneCall("message_received").withUnsignedIntParameter("status", NETRET_EXCHANGE);
   	uv_run(loop, UV_RUN_NOWAIT);
   	ObjectPolyFinish((Object)conn);
   	fd = -1;
           EnableIoMock(false);
   }
   
   ```
   
   Triggering timeout when sending a tiny packet is very difficult (if not possible) in reality, even with the help of expensive network emulation hardware. But with link-time test double, it's easy to inject error into any C library call. Also the test case is extremely fast to execute, since we use fake clock to avoid waiting 10 seconds. High line/branch coverage (like sqlite) is not difficult to achieve.
   
   However, such powerful testing technique is not without its price:
   
   * 1000 LOC `AlarmUploader` takes 3300 LOC to test.
   * Such tests are not written against API, they are completely whitebox.
   * Bad readability as a consequence. API test case can be used as demo while the above test case cannot.
   
   We have to take extra care of the interplay between malloc/calloc/pthread test double and Google Address Sanitizer so that they can work together. I'll prepare a gist for malloc/calloc if Celix wants to take this whitebox way of testing. @pnoltes 




-- 
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-unsubscribe@celix.apache.org

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

[GitHub] [celix] codecov-commenter commented on pull request #392: Tracker improvements preliminary

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #392:
URL: https://github.com/apache/celix/pull/392#issuecomment-1019932519


   # [Codecov](https://codecov.io/gh/apache/celix/pull/392?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#392](https://codecov.io/gh/apache/celix/pull/392?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (3b65e35) into [master](https://codecov.io/gh/apache/celix/commit/70d6219ee48814e9f737b231d521da76e382b4d8?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (70d6219) will **decrease** coverage by `0.11%`.
   > The diff coverage is `86.93%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/celix/pull/392/graphs/tree.svg?width=650&height=150&src=pr&token=JdsiThga8P&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/celix/pull/392?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #392      +/-   ##
   ==========================================
   - Coverage   72.87%   72.76%   -0.12%     
   ==========================================
     Files         204      205       +1     
     Lines       31325    31293      -32     
   ==========================================
   - Hits        22827    22769      -58     
   - Misses       8498     8524      +26     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/celix/pull/392?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [libs/framework/src/framework.c](https://codecov.io/gh/apache/celix/pull/392/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bGlicy9mcmFtZXdvcmsvc3JjL2ZyYW1ld29yay5j) | `77.55% <ø> (+0.12%)` | :arrow_up: |
   | [libs/framework/src/bundle\_context.c](https://codecov.io/gh/apache/celix/pull/392/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bGlicy9mcmFtZXdvcmsvc3JjL2J1bmRsZV9jb250ZXh0LmM=) | `75.96% <73.91%> (-0.35%)` | :arrow_down: |
   | [libs/framework/src/service\_tracker.c](https://codecov.io/gh/apache/celix/pull/392/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bGlicy9mcmFtZXdvcmsvc3JjL3NlcnZpY2VfdHJhY2tlci5j) | `73.47% <76.19%> (+3.39%)` | :arrow_up: |
   | [libs/framework/src/service\_reference.c](https://codecov.io/gh/apache/celix/pull/392/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bGlicy9mcmFtZXdvcmsvc3JjL3NlcnZpY2VfcmVmZXJlbmNlLmM=) | `49.53% <84.61%> (-0.69%)` | :arrow_down: |
   | [libs/utils/include/celix\_ref.h](https://codecov.io/gh/apache/celix/pull/392/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bGlicy91dGlscy9pbmNsdWRlL2NlbGl4X3JlZi5o) | `86.66% <86.66%> (ø)` | |
   | [libs/framework/src/service\_registry.c](https://codecov.io/gh/apache/celix/pull/392/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bGlicy9mcmFtZXdvcmsvc3JjL3NlcnZpY2VfcmVnaXN0cnkuYw==) | `74.57% <98.18%> (-0.37%)` | :arrow_down: |
   | [...e\_services/topology\_manager/src/topology\_manager.c](https://codecov.io/gh/apache/celix/pull/392/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YnVuZGxlcy9yZW1vdGVfc2VydmljZXMvdG9wb2xvZ3lfbWFuYWdlci9zcmMvdG9wb2xvZ3lfbWFuYWdlci5j) | `83.43% <100.00%> (+0.06%)` | :arrow_up: |
   | [libs/framework/src/service\_registration.c](https://codecov.io/gh/apache/celix/pull/392/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bGlicy9mcmFtZXdvcmsvc3JjL3NlcnZpY2VfcmVnaXN0cmF0aW9uLmM=) | `86.75% <100.00%> (-0.52%)` | :arrow_down: |
   | [libs/utils/src/array\_list.c](https://codecov.io/gh/apache/celix/pull/392/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bGlicy91dGlscy9zcmMvYXJyYXlfbGlzdC5j) | `87.57% <100.00%> (ø)` | |
   | [...ubsub\_serializer\_json/src/pubsub\_serializer\_impl.c](https://codecov.io/gh/apache/celix/pull/392/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YnVuZGxlcy9wdWJzdWIvcHVic3ViX3NlcmlhbGl6ZXJfanNvbi9zcmMvcHVic3ViX3NlcmlhbGl6ZXJfaW1wbC5j) | `35.63% <0.00%> (-18.91%)` | :arrow_down: |
   | ... and [4 more](https://codecov.io/gh/apache/celix/pull/392/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/celix/pull/392?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/celix/pull/392?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [70d6219...3b65e35](https://codecov.io/gh/apache/celix/pull/392?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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-unsubscribe@celix.apache.org

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

[GitHub] [celix] pnoltes commented on a change in pull request #392: Tracker improvements preliminary

Posted by GitBox <gi...@apache.org>.
pnoltes commented on a change in pull request #392:
URL: https://github.com/apache/celix/pull/392#discussion_r791031367



##########
File path: libs/framework/src/bundle_context.c
##########
@@ -76,7 +76,7 @@ celix_status_t bundleContext_create(framework_pt framework, celix_framework_logg
 
         }
 	}
-
+    //FIXME: context == NULL?

Review comment:
       Do you also know if there is a good way to check (unit test) if the code correctly handles NULL returns from malloc/calloc? 
   IMO this is the primary reason we dropped checking malloc/calloc returns. 




-- 
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-unsubscribe@celix.apache.org

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

[GitHub] [celix] pnoltes commented on a change in pull request #392: Tracker improvements preliminary

Posted by GitBox <gi...@apache.org>.
pnoltes commented on a change in pull request #392:
URL: https://github.com/apache/celix/pull/392#discussion_r791031367



##########
File path: libs/framework/src/bundle_context.c
##########
@@ -76,7 +76,7 @@ celix_status_t bundleContext_create(framework_pt framework, celix_framework_logg
 
         }
 	}
-
+    //FIXME: context == NULL?

Review comment:
       Do you also know if there is a good way to check (unit test) if the code correctly handles NULL returns from malloc/calloc? 
   IMO this is the primary reason we dropped checking malloc/calloc returns. 




-- 
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-unsubscribe@celix.apache.org

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

[GitHub] [celix] PengZheng commented on a change in pull request #392: Tracker improvements preliminary

Posted by GitBox <gi...@apache.org>.
PengZheng commented on a change in pull request #392:
URL: https://github.com/apache/celix/pull/392#discussion_r789424559



##########
File path: libs/utils/include/celix_build_assert.h
##########
@@ -0,0 +1,26 @@
+/*
+ * 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.
+ */
+
+#ifndef CELIX_CELIX_BUILD_ASSERT_H
+#define CELIX_CELIX_BUILD_ASSERT_H
+
+#define BUILD_ASSERT(cond) \

Review comment:
       Should I add CELIX_ prefix?




-- 
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-unsubscribe@celix.apache.org

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

[GitHub] [celix] pnoltes commented on a change in pull request #392: Tracker improvements preliminary

Posted by GitBox <gi...@apache.org>.
pnoltes commented on a change in pull request #392:
URL: https://github.com/apache/celix/pull/392#discussion_r790317824



##########
File path: CMakeLists.txt
##########
@@ -44,7 +44,7 @@ endif ()
 set(ENABLE_MORE_WARNINGS OFF)
 
 # Set C specific flags
-set(CMAKE_C_FLAGS "-D_GNU_SOURCE -std=gnu99 -fPIC ${CMAKE_C_FLAGS}")
+set(CMAKE_C_FLAGS "-D_GNU_SOURCE -std=gnu11 -fPIC ${CMAKE_C_FLAGS}")

Review comment:
       Note: that this will update the C requirements for Celix from gnu99 to gnu11. 
   
   I have no issue with this, but maybe it good to get some broader feedback on this.
   @rlenferink , @rbulter , @ErjanAltena and @stegemr Do any of you have any issues updating the Celix C standard from gnu99 to gnu11? . 
   
   ... And if we have no issues, should we also introduce the use of `#pragma once` for C code?




-- 
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-unsubscribe@celix.apache.org

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

[GitHub] [celix] PengZheng commented on a change in pull request #392: Tracker improvements preliminary

Posted by GitBox <gi...@apache.org>.
PengZheng commented on a change in pull request #392:
URL: https://github.com/apache/celix/pull/392#discussion_r791780927



##########
File path: libs/framework/src/bundle_context.c
##########
@@ -76,7 +76,7 @@ celix_status_t bundleContext_create(framework_pt framework, celix_framework_logg
 
         }
 	}
-
+    //FIXME: context == NULL?

Review comment:
       I guess you are talking about injecting errors into libc or any 3rd party library function call when doing unit test, right? 
   The standard mocking method in C++  is to provide mock implementation of an abstract interface. The C equivalent is using function pointers:
   
   So instead of 
   ```C
   int FormatOutput(const char *, ...);
   ```
   we put the following in the header file:
   ```C
   extern int (*FormatOutput)(const char *, ...);
   ```
   
   Though both of them support runtime-bound test doubles very well, careless use can lead to lots of unnecessary abstract classes/function pointers, which could harm both performance and code readability considerably.  In this particular case (libc/3rd party C libraries), these runtime test doubles are pretty awkward to use,  i.e. we have to re-exposing functions as function pointers instead of using header file directly.
   
   Instead, I use link-time substitution. It does not require any wierd wrappers in production codes. We still need wrappers, but they only appear in testing code. It permits testing corner cases which is almost impossible to cover in system test like the following:
   
   ```C
   TEST(AlarmUploader, HeartbeatTimeoutWhenSending)
   {
           NETRET_HEADER header;
   	expectRecv = true;
           //enable link-time test doubles for clock_gettime and gettimeofday
           //if disabled, they will call the libc's implementation
   	EnableClockMock(true);
   	LONGS_EQUAL(EDA_OK, AlarmUploaderInit(conn, module, &attr));
   	LONGS_EQUAL(EDA_OK, AlarmUploaderConnect(conn, NULL, NULL));
   	//enable IO mock to substitute write and writev, which libuv uses for socket IO
           EnableIoMock(true);
   	//limit each write/writev invocation can send out at most 1 byte
   	//so that the transportation of tiny heartbeat packet (< 100 bytes) cannot be completed in one uv_run call
   	SetIoMockMaxBytesPerWrite(1);
           // let 5.001 seconds pass in our fake clock to trigger 5 seconds timeout
   	ClockAdvance(5001); 
           // actually trigger libuv timer with heartbeat pending
   	uv_run(loop, UV_RUN_NOWAIT);
           // disable IO mock, so that libuv send out packet at normal speed
   	EnableIoMock(false);
   	ClockAdvance(5000);
   	ExpectNetRetHeader(&header, NETRET_QUALIFIED);
   	mock().expectOneCall("message_received").withUnsignedIntParameter("status", NETRET_EXCHANGE);
   	mock().expectOneCall("message_received").withUnsignedIntParameter("status", NETRET_EXCHANGE);
   	uv_run(loop, UV_RUN_NOWAIT);
   	ObjectPolyFinish((Object)conn);
   	fd = -1;
           EnableIoMock(false);
   }
   
   ```
   
   Triggering timeout when sending a tiny packet is very difficult (if not possible) in reality, even with the help of expensive network emulation hardware. But with link-time test double, it's easy to inject various errors into any C library call. Also the test case is extremely fast to execute, since we use fake clock to avoid waiting 10 seconds. High line/branch coverage (like sqlite) is not difficult to achieve.
   
   However, such powerful testing technique is not without its price:
   
   * 1000 LOC `AlarmUploader` takes 3300 LOC to test.
   * Such tests are not written against API, they are completely whitebox.
   * Bad readability as a consequence. API test case can be used as demo while the above test case cannot.
   
   We have to take extra care of the interplay between malloc/calloc/pthread test double and Google Address Sanitizer so that they can work together. I'll prepare a gist for malloc/calloc if Celix wants to take this whitebox way of testing. @pnoltes 




-- 
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-unsubscribe@celix.apache.org

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