PengZheng commented on code in PR #711:
URL: https://github.com/apache/celix/pull/711#discussion_r1439133798
##########
bundles/remote_services/discovery_common/include/endpoint_discovery_poller.h:
##########
@@ -55,6 +55,6 @@ celix_status_t
endpointDiscoveryPoller_destroy(endpoint_discovery_poller_t *poll
celix_status_t
endpointDiscoveryPoller_addDiscoveryEndpoint(endpoint_discovery_poller_t
*poller, char *url);
celix_status_t
endpointDiscoveryPoller_removeDiscoveryEndpoint(endpoint_discovery_poller_t
*poller, char *url);
-celix_status_t
endpointDiscoveryPoller_getDiscoveryEndpoints(endpoint_discovery_poller_t
*poller, array_list_pt urls);
+celix_status_t
endpointDiscoveryPoller_getDiscoveryEndpoints(endpoint_discovery_poller_t
*poller, celix_array_list_t* urls);
Review Comment:
Missing `#include "celix_array_list.h"`.
##########
bundles/remote_services/discovery_common/src/endpoint_discovery_poller.c:
##########
@@ -183,111 +184,118 @@ celix_status_t
endpointDiscoveryPoller_getDiscoveryEndpoints(endpoint_discovery_
/**
* Adds a new endpoint URL to the list of polled endpoints.
*/
-celix_status_t
endpointDiscoveryPoller_addDiscoveryEndpoint(endpoint_discovery_poller_t
*poller, char *url) {
- celix_status_t status;
+celix_status_t
endpointDiscoveryPoller_addDiscoveryEndpoint(endpoint_discovery_poller_t*
poller, char* url) {
+ celix_status_t status;
- status = celixThreadMutex_lock(&(poller)->pollerLock);
- if (status != CELIX_SUCCESS) {
- return CELIX_BUNDLE_EXCEPTION;
- }
+ status = celixThreadMutex_lock(&(poller)->pollerLock);
+ if (status != CELIX_SUCCESS) {
+ return CELIX_BUNDLE_EXCEPTION;
+ }
- // Avoid memory leaks when adding an already existing URL...
- array_list_pt endpoints = hashMap_get(poller->entries, url);
- if (endpoints == NULL) {
- status =
arrayList_createWithEquals(endpointDiscoveryPoller_endpointDescriptionEquals,
&endpoints);
+ // Avoid memory leaks when adding an already existing URL...
+ celix_array_list_t* endpoints = hashMap_get(poller->entries, url);
+ if (endpoints == NULL) {
+ endpoints =
celix_arrayList_createWithEquals(endpointDiscoveryPoller_endpointDescriptionEquals);
- if (status == CELIX_SUCCESS) {
+ if (endpoints) {
celix_logHelper_debug(*poller->loghelper, "ENDPOINT_POLLER: add
new discovery endpoint with url %s", url);
- hashMap_put(poller->entries, strdup(url), endpoints);
- endpointDiscoveryPoller_poll(poller, url, endpoints);
- }
- }
+ hashMap_put(poller->entries, strdup(url), endpoints);
+ endpointDiscoveryPoller_poll(poller, url, endpoints);
+ }
+ }
- status = celixThreadMutex_unlock(&poller->pollerLock);
+ status = celixThreadMutex_unlock(&poller->pollerLock);
- return status;
+ return status;
}
/**
* Removes an endpoint URL from the list of polled endpoints.
*/
-celix_status_t
endpointDiscoveryPoller_removeDiscoveryEndpoint(endpoint_discovery_poller_t
*poller, char *url) {
- celix_status_t status = CELIX_SUCCESS;
-
- if (celixThreadMutex_lock(&poller->pollerLock) != CELIX_SUCCESS) {
- status = CELIX_BUNDLE_EXCEPTION;
- } else {
- hash_map_entry_pt entry = hashMap_getEntry(poller->entries,
url);
-
- if (entry == NULL) {
- celix_logHelper_debug(*poller->loghelper, "ENDPOINT_POLLER: There
was no entry found belonging to url %s - maybe already removed?", url);
- } else {
- char* origKey = hashMapEntry_getKey(entry);
+celix_status_t
endpointDiscoveryPoller_removeDiscoveryEndpoint(endpoint_discovery_poller_t*
poller, char* url) {
+ celix_status_t status = CELIX_SUCCESS;
+
+ if (celixThreadMutex_lock(&poller->pollerLock) != CELIX_SUCCESS) {
+ status = CELIX_BUNDLE_EXCEPTION;
+ } else {
+ hash_map_entry_pt entry = hashMap_getEntry(poller->entries, url);
+
+ if (entry == NULL) {
+ celix_logHelper_debug(
+ *poller->loghelper,
+ "ENDPOINT_POLLER: There was no entry found belonging to url %s
- maybe already removed?",
+ url);
+ } else {
+ char* origKey = hashMapEntry_getKey(entry);
celix_logHelper_debug(*poller->loghelper, "ENDPOINT_POLLER: remove
discovery endpoint with url %s", url);
- array_list_pt entries = hashMap_remove(poller->entries,
url);
+ celix_array_list_t* entries = hashMap_remove(poller->entries, url);
- if (entries != NULL) {
- for (unsigned int i = arrayList_size(entries);
i > 0; i--) {
- endpoint_description_t *endpoint =
arrayList_get(entries, i - 1);
-
discovery_removeDiscoveredEndpoint(poller->discovery, endpoint);
- arrayList_remove(entries, i - 1);
- endpointDescription_destroy(endpoint);
- }
- arrayList_destroy(entries);
- }
+ if (entries != NULL) {
+ for (unsigned int i = celix_arrayList_size(entries); i > 0;
i--) {
+ endpoint_description_t* endpoint =
celix_arrayList_get(entries, i - 1);
+ discovery_removeDiscoveredEndpoint(poller->discovery,
endpoint);
+ celix_arrayList_removeAt(entries, i - 1);
+ endpointDescription_destroy(endpoint);
+ }
+ celix_arrayList_destroy(entries);
+ }
- free(origKey);
- }
- status = celixThreadMutex_unlock(&poller->pollerLock);
- }
+ free(origKey);
+ }
+ status = celixThreadMutex_unlock(&poller->pollerLock);
+ }
- return status;
+ return status;
}
-
-
-
-celix_status_t endpointDiscoveryPoller_poll(endpoint_discovery_poller_t
*poller, char *url, array_list_pt currentEndpoints) {
- celix_status_t status;
- array_list_pt updatedEndpoints = NULL;
-
- // create an arraylist with a custom equality test to ensure we can
find endpoints properly...
-
arrayList_createWithEquals(endpointDiscoveryPoller_endpointDescriptionEquals,
&updatedEndpoints);
- status = endpointDiscoveryPoller_getEndpoints(poller, url,
&updatedEndpoints);
-
- if (status == CELIX_SUCCESS) {
- if (updatedEndpoints != NULL) {
- for (unsigned int i = arrayList_size(currentEndpoints);
i > 0; i--) {
- endpoint_description_t *endpoint =
arrayList_get(currentEndpoints, i - 1);
-
- if (!arrayList_contains(updatedEndpoints,
endpoint)) {
- status =
discovery_removeDiscoveredEndpoint(poller->discovery, endpoint);
- arrayList_remove(currentEndpoints, i -
1);
- endpointDescription_destroy(endpoint);
- }
- }
-
- for (int i = arrayList_size(updatedEndpoints); i > 0;
i--) {
- endpoint_description_t *endpoint =
arrayList_remove(updatedEndpoints, 0);
-
- if (!arrayList_contains(currentEndpoints,
endpoint)) {
- arrayList_add(currentEndpoints,
endpoint);
- status =
discovery_addDiscoveredEndpoint(poller->discovery, endpoint);
- } else {
- endpointDescription_destroy(endpoint);
-
- }
- }
- }
- }
-
- if (updatedEndpoints != NULL) {
- arrayList_destroy(updatedEndpoints);
- }
-
- return status;
+celix_status_t
+endpointDiscoveryPoller_poll(endpoint_discovery_poller_t* poller, char* url,
celix_array_list_t* currentEndpoints) {
+ celix_status_t status;
+ celix_array_list_t* updatedEndpoints = NULL;
+
+ // create an arraylist with a custom equality test to ensure we can find
endpoints properly...
+ updatedEndpoints =
celix_arrayList_createWithEquals(endpointDiscoveryPoller_endpointDescriptionEquals);
Review Comment:
If `updatedEndpoints == NULL`, we need to return error early. Otherwise,
`endpointDiscoveryPoller_getEndpoints` will call normal
`celix_arrayList_create` to try to create it again, which is inconsistent with
`celix_arrayList_createWithEquals`.
##########
bundles/remote_services/topology_manager/src/scope.c:
##########
@@ -150,12 +153,15 @@ celix_status_t tm_removeImportScope(void *handle, char
*filter) {
}
if (celixThreadMutex_lock(&scope->importScopeLock) == CELIX_SUCCESS) {
- int index = arrayList_indexOf(scope->importScopes, new);
- filter_pt present = (filter_pt) arrayList_get(scope->importScopes,
index);
+ celix_array_list_entry_t entry;
+ memset(&entry, 0, sizeof(entry));
+ entry.voidPtrVal = new;
+ int index = celix_arrayList_indexOf(scope->importScopes, entry);
Review Comment:
The above remark also applies here.
##########
bundles/remote_services/discovery_common/src/endpoint_discovery_poller.c:
##########
@@ -183,111 +184,118 @@ celix_status_t
endpointDiscoveryPoller_getDiscoveryEndpoints(endpoint_discovery_
/**
* Adds a new endpoint URL to the list of polled endpoints.
*/
-celix_status_t
endpointDiscoveryPoller_addDiscoveryEndpoint(endpoint_discovery_poller_t
*poller, char *url) {
- celix_status_t status;
+celix_status_t
endpointDiscoveryPoller_addDiscoveryEndpoint(endpoint_discovery_poller_t*
poller, char* url) {
+ celix_status_t status;
- status = celixThreadMutex_lock(&(poller)->pollerLock);
- if (status != CELIX_SUCCESS) {
- return CELIX_BUNDLE_EXCEPTION;
- }
+ status = celixThreadMutex_lock(&(poller)->pollerLock);
+ if (status != CELIX_SUCCESS) {
+ return CELIX_BUNDLE_EXCEPTION;
+ }
- // Avoid memory leaks when adding an already existing URL...
- array_list_pt endpoints = hashMap_get(poller->entries, url);
- if (endpoints == NULL) {
- status =
arrayList_createWithEquals(endpointDiscoveryPoller_endpointDescriptionEquals,
&endpoints);
+ // Avoid memory leaks when adding an already existing URL...
+ celix_array_list_t* endpoints = hashMap_get(poller->entries, url);
+ if (endpoints == NULL) {
+ endpoints =
celix_arrayList_createWithEquals(endpointDiscoveryPoller_endpointDescriptionEquals);
- if (status == CELIX_SUCCESS) {
+ if (endpoints) {
celix_logHelper_debug(*poller->loghelper, "ENDPOINT_POLLER: add
new discovery endpoint with url %s", url);
- hashMap_put(poller->entries, strdup(url), endpoints);
- endpointDiscoveryPoller_poll(poller, url, endpoints);
- }
- }
+ hashMap_put(poller->entries, strdup(url), endpoints);
+ endpointDiscoveryPoller_poll(poller, url, endpoints);
+ }
+ }
- status = celixThreadMutex_unlock(&poller->pollerLock);
+ status = celixThreadMutex_unlock(&poller->pollerLock);
- return status;
+ return status;
}
/**
* Removes an endpoint URL from the list of polled endpoints.
*/
-celix_status_t
endpointDiscoveryPoller_removeDiscoveryEndpoint(endpoint_discovery_poller_t
*poller, char *url) {
- celix_status_t status = CELIX_SUCCESS;
-
- if (celixThreadMutex_lock(&poller->pollerLock) != CELIX_SUCCESS) {
- status = CELIX_BUNDLE_EXCEPTION;
- } else {
- hash_map_entry_pt entry = hashMap_getEntry(poller->entries,
url);
-
- if (entry == NULL) {
- celix_logHelper_debug(*poller->loghelper, "ENDPOINT_POLLER: There
was no entry found belonging to url %s - maybe already removed?", url);
- } else {
- char* origKey = hashMapEntry_getKey(entry);
+celix_status_t
endpointDiscoveryPoller_removeDiscoveryEndpoint(endpoint_discovery_poller_t*
poller, char* url) {
+ celix_status_t status = CELIX_SUCCESS;
+
+ if (celixThreadMutex_lock(&poller->pollerLock) != CELIX_SUCCESS) {
+ status = CELIX_BUNDLE_EXCEPTION;
+ } else {
+ hash_map_entry_pt entry = hashMap_getEntry(poller->entries, url);
+
+ if (entry == NULL) {
+ celix_logHelper_debug(
+ *poller->loghelper,
+ "ENDPOINT_POLLER: There was no entry found belonging to url %s
- maybe already removed?",
+ url);
+ } else {
+ char* origKey = hashMapEntry_getKey(entry);
celix_logHelper_debug(*poller->loghelper, "ENDPOINT_POLLER: remove
discovery endpoint with url %s", url);
- array_list_pt entries = hashMap_remove(poller->entries,
url);
+ celix_array_list_t* entries = hashMap_remove(poller->entries, url);
- if (entries != NULL) {
- for (unsigned int i = arrayList_size(entries);
i > 0; i--) {
- endpoint_description_t *endpoint =
arrayList_get(entries, i - 1);
-
discovery_removeDiscoveredEndpoint(poller->discovery, endpoint);
- arrayList_remove(entries, i - 1);
- endpointDescription_destroy(endpoint);
- }
- arrayList_destroy(entries);
- }
+ if (entries != NULL) {
+ for (unsigned int i = celix_arrayList_size(entries); i > 0;
i--) {
+ endpoint_description_t* endpoint =
celix_arrayList_get(entries, i - 1);
+ discovery_removeDiscoveredEndpoint(poller->discovery,
endpoint);
+ celix_arrayList_removeAt(entries, i - 1);
+ endpointDescription_destroy(endpoint);
+ }
+ celix_arrayList_destroy(entries);
+ }
- free(origKey);
- }
- status = celixThreadMutex_unlock(&poller->pollerLock);
- }
+ free(origKey);
+ }
+ status = celixThreadMutex_unlock(&poller->pollerLock);
+ }
- return status;
+ return status;
}
-
-
-
-celix_status_t endpointDiscoveryPoller_poll(endpoint_discovery_poller_t
*poller, char *url, array_list_pt currentEndpoints) {
- celix_status_t status;
- array_list_pt updatedEndpoints = NULL;
-
- // create an arraylist with a custom equality test to ensure we can
find endpoints properly...
-
arrayList_createWithEquals(endpointDiscoveryPoller_endpointDescriptionEquals,
&updatedEndpoints);
- status = endpointDiscoveryPoller_getEndpoints(poller, url,
&updatedEndpoints);
-
- if (status == CELIX_SUCCESS) {
- if (updatedEndpoints != NULL) {
- for (unsigned int i = arrayList_size(currentEndpoints);
i > 0; i--) {
- endpoint_description_t *endpoint =
arrayList_get(currentEndpoints, i - 1);
-
- if (!arrayList_contains(updatedEndpoints,
endpoint)) {
- status =
discovery_removeDiscoveredEndpoint(poller->discovery, endpoint);
- arrayList_remove(currentEndpoints, i -
1);
- endpointDescription_destroy(endpoint);
- }
- }
-
- for (int i = arrayList_size(updatedEndpoints); i > 0;
i--) {
- endpoint_description_t *endpoint =
arrayList_remove(updatedEndpoints, 0);
-
- if (!arrayList_contains(currentEndpoints,
endpoint)) {
- arrayList_add(currentEndpoints,
endpoint);
- status =
discovery_addDiscoveredEndpoint(poller->discovery, endpoint);
- } else {
- endpointDescription_destroy(endpoint);
-
- }
- }
- }
- }
-
- if (updatedEndpoints != NULL) {
- arrayList_destroy(updatedEndpoints);
- }
-
- return status;
+celix_status_t
+endpointDiscoveryPoller_poll(endpoint_discovery_poller_t* poller, char* url,
celix_array_list_t* currentEndpoints) {
+ celix_status_t status;
+ celix_array_list_t* updatedEndpoints = NULL;
+
+ // create an arraylist with a custom equality test to ensure we can find
endpoints properly...
+ updatedEndpoints =
celix_arrayList_createWithEquals(endpointDiscoveryPoller_endpointDescriptionEquals);
+ status = endpointDiscoveryPoller_getEndpoints(poller, url,
&updatedEndpoints);
+
+ if (updatedEndpoints && status == CELIX_SUCCESS) {
+ if (updatedEndpoints != NULL) {
Review Comment:
Unnecessary guard.
##########
libs/utils/src/ip_utils.c:
##########
@@ -16,155 +16,207 @@
* specific language governing permissions and limitations
* under the License.
*/
-/**
- * ip_utils.c
- *
- * \date Jun 24, 2019
- * \author <a href="mailto:dev@celix.apache.org";>Apache Celix Project
Team</a>
- * \copyright Apache License, Version 2.0
- */
-#include "ip_utils.h"
+#include "celix_ip_utils.h"
+
+#include "celix_err.h"
+#include "celix_stdlib_cleanup.h"
#include "celix_utils.h"
+#include "celix_convert_utils.h"
+#include <arpa/inet.h>
+#include <assert.h>
#include <errno.h>
+#include <ifaddrs.h>
+#include <limits.h>
+#include <math.h>
+#include <netdb.h>
+#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>
-#include <stdbool.h>
#include <string.h>
-#include <math.h>
-
-#include <arpa/inet.h>
#include <sys/socket.h>
-#include <netdb.h>
-#include <ifaddrs.h>
-#include <unistd.h>
-unsigned int ipUtils_ipToUnsignedInt(char *ip) {
- unsigned int ipAsUint = 0;
+uint32_t celix_utils_convertIpToUint(const char* ip, bool* converted) {
Review Comment:
It seems that we can use `inet_pton` instead of
`celix_utils_convertIpToUint`.
For example, the following implementation passes all non-error-injection
tests.
```C
uint32_t celix_utils_convertIpToUint(const char* ip, bool* converted) {
if (converted) {
*converted = false;
}
struct in_addr result;
uint32_t ipAsUint = 0;
if(inet_pton(AF_INET, ip, &result)) {
ipAsUint = ntohl(result.s_addr);
if (converted) {
*converted = true;
}
} else {
celix_err_pushf("Failed to convert IP address %s to unsigned int",
ip);
}
return ipAsUint;
}
```
##########
libs/utils/src/ip_utils.c:
##########
@@ -16,155 +16,207 @@
* specific language governing permissions and limitations
* under the License.
*/
-/**
- * ip_utils.c
- *
- * \date Jun 24, 2019
- * \author <a href="mailto:dev@celix.apache.org";>Apache Celix Project
Team</a>
- * \copyright Apache License, Version 2.0
- */
-#include "ip_utils.h"
+#include "celix_ip_utils.h"
+
+#include "celix_err.h"
+#include "celix_stdlib_cleanup.h"
#include "celix_utils.h"
+#include "celix_convert_utils.h"
+#include <arpa/inet.h>
+#include <assert.h>
#include <errno.h>
+#include <ifaddrs.h>
+#include <limits.h>
+#include <math.h>
+#include <netdb.h>
+#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>
-#include <stdbool.h>
#include <string.h>
-#include <math.h>
-
-#include <arpa/inet.h>
#include <sys/socket.h>
-#include <netdb.h>
-#include <ifaddrs.h>
-#include <unistd.h>
-unsigned int ipUtils_ipToUnsignedInt(char *ip) {
- unsigned int ipAsUint = 0;
+uint32_t celix_utils_convertIpToUint(const char* ip, bool* converted) {
+ if (converted) {
+ *converted = false;
+ }
- char *partOfIp = NULL, *savePtr = NULL;
- char *input = strdup(ip); // Make a copy because otherwise strtok_r
manipulates the input string
- partOfIp = strtok_r(input, ".\0", &savePtr); ipAsUint += strtoul(partOfIp,
NULL, 10) * (unsigned int) pow(256, 3);
- partOfIp = strtok_r(NULL, ".\0", &savePtr); ipAsUint += strtoul(partOfIp,
NULL, 10) * (unsigned int) pow(256, 2);
- partOfIp = strtok_r(NULL, ".\0", &savePtr); ipAsUint += strtoul(partOfIp,
NULL, 10) * (unsigned int) pow(256, 1);
- partOfIp = strtok_r(NULL, ".\0", &savePtr); ipAsUint += strtoul(partOfIp,
NULL, 10) * (unsigned int) pow(256, 0);
- free(input);
+ // copy for strtok_r
+ celix_autofree char* input = celix_utils_strdup(ip);
+ if (!input) {
+ celix_err_push("Failed to duplicate input string for IP conversion");
+ return 0;
+ }
+ uint32_t ipAsUint = 0;
+ char* partOfIp = NULL;
+ char* savePtr = NULL;
+ int count = 0;
+ while ((partOfIp = strtok_r(partOfIp == NULL ? input : NULL, ".\0",
&savePtr)) != NULL) {
+ if (count > 3) {
+ celix_err_pushf("Failed to convert IP address %s to unsigned int,
to many parts", ip);
+ return 0;
+ }
+ bool longConverted = false;
+ long partAsLong = celix_utils_convertStringToLong(partOfIp, ULONG_MAX,
&longConverted);
+ if (!longConverted) {
+ celix_err_pushf("Failed to convert IP address %s to unsigned int,
part `%s` is not a number", ip, partOfIp);
+ return 0;
+ } else if (partAsLong > 255 || partAsLong < 0) {
+ celix_err_pushf("Failed to convert IP address %s to unsigned int,
part `%s` is out of range", ip, partOfIp);
+ return 0;
+ }
+ ipAsUint += (uint32_t)(partAsLong * powl(256, 3 - count++));
+ }
+
+ if (converted) {
+ *converted = true;
+ }
return ipAsUint;
}
-char *ipUtils_unsignedIntToIp(unsigned int ip) {
- char *ipStr = calloc(16, sizeof(char));
+char* celix_utils_convertUintToIp(uint32_t ip) {
+ char* ipStr = calloc(16, sizeof(char));
+ if (!ipStr) {
+ celix_err_push("Failed to allocate memory for IP address string");
+ return NULL;
+ }
- int ipPart1 = ip / (int) pow(256, 3); ip -= ipPart1 * (int) pow(256, 3);
- int ipPart2 = ip / (int) pow(256, 2); ip -= ipPart2 * (int) pow(256, 2);
- int ipPart3 = ip / (int) pow(256, 1); ip -= ipPart3 * (int) pow(256, 1);
- int ipPart4 = ip / (int) pow(256, 0);
+ int64_t ipPart1 = ip / (int64_t)pow(256, 3);
+ ip -= ipPart1 * (int64_t)pow(256, 3);
+ int64_t ipPart2 = ip / (int64_t)pow(256, 2);
+ ip -= ipPart2 * (int64_t)pow(256, 2);
+ int64_t ipPart3 = ip / (int64_t)pow(256, 1);
+ ip -= ipPart3 * (int64_t)pow(256, 1);
+ int64_t ipPart4 = ip / (int64_t)pow(256, 0);
- snprintf(ipStr, 16, "%d.%d.%d.%d", ipPart1, ipPart2, ipPart3, ipPart4);
+ snprintf(ipStr, 16, "%i.%i.%i.%i", (int)ipPart1, (int)ipPart2,
(int)ipPart3, (int)ipPart4);
return ipStr;
}
-unsigned int ipUtils_prefixToBitmask(unsigned int prefix) {
- return (0xFFFFFFFF << (32 - prefix)) & 0xFFFFFFFF;
+uint32_t celix_utils_ipPrefixLengthToBitmask(int prefix) {
+ if (prefix > 32 || prefix <= 0) {
+ return 0;
+ }
+ return (uint32_t )((0xFFFFFFFF << (32 - prefix)) & 0xFFFFFFFF);
}
-int ipUtils_netmaskToPrefix(const char *netmask) {
+int celix_utils_ipNetmaskToPrefixLength(const char* netmask) {
// Convert netmask to in_addr object
struct in_addr in;
int ret = inet_pton(AF_INET, netmask, &in);
if (ret != 1) {
+ celix_err_pushf("Failed to convert netmask %s to in_addr object",
netmask);
return -1;
}
// Now convert the mask to a prefix
int prefix = 0;
- bool processed_one = false;
- unsigned int i = ntohl(in.s_addr);
+ uint32_t i = ntohl(in.s_addr);
while (i > 0) {
if (i & 1) {
prefix++;
Review Comment:
Could the loop be replaced by `__builtin_popcount`, which is supported by
both clang and gcc.
##########
libs/utils/src/ip_utils.c:
##########
@@ -16,155 +16,207 @@
* specific language governing permissions and limitations
* under the License.
*/
-/**
- * ip_utils.c
- *
- * \date Jun 24, 2019
- * \author <a href="mailto:dev@celix.apache.org";>Apache Celix Project
Team</a>
- * \copyright Apache License, Version 2.0
- */
-#include "ip_utils.h"
+#include "celix_ip_utils.h"
+
+#include "celix_err.h"
+#include "celix_stdlib_cleanup.h"
#include "celix_utils.h"
+#include "celix_convert_utils.h"
+#include <arpa/inet.h>
+#include <assert.h>
#include <errno.h>
+#include <ifaddrs.h>
+#include <limits.h>
+#include <math.h>
+#include <netdb.h>
+#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>
-#include <stdbool.h>
#include <string.h>
-#include <math.h>
-
-#include <arpa/inet.h>
#include <sys/socket.h>
-#include <netdb.h>
-#include <ifaddrs.h>
-#include <unistd.h>
-unsigned int ipUtils_ipToUnsignedInt(char *ip) {
- unsigned int ipAsUint = 0;
+uint32_t celix_utils_convertIpToUint(const char* ip, bool* converted) {
+ if (converted) {
+ *converted = false;
+ }
- char *partOfIp = NULL, *savePtr = NULL;
- char *input = strdup(ip); // Make a copy because otherwise strtok_r
manipulates the input string
- partOfIp = strtok_r(input, ".\0", &savePtr); ipAsUint += strtoul(partOfIp,
NULL, 10) * (unsigned int) pow(256, 3);
- partOfIp = strtok_r(NULL, ".\0", &savePtr); ipAsUint += strtoul(partOfIp,
NULL, 10) * (unsigned int) pow(256, 2);
- partOfIp = strtok_r(NULL, ".\0", &savePtr); ipAsUint += strtoul(partOfIp,
NULL, 10) * (unsigned int) pow(256, 1);
- partOfIp = strtok_r(NULL, ".\0", &savePtr); ipAsUint += strtoul(partOfIp,
NULL, 10) * (unsigned int) pow(256, 0);
- free(input);
+ // copy for strtok_r
+ celix_autofree char* input = celix_utils_strdup(ip);
+ if (!input) {
+ celix_err_push("Failed to duplicate input string for IP conversion");
+ return 0;
+ }
+ uint32_t ipAsUint = 0;
+ char* partOfIp = NULL;
+ char* savePtr = NULL;
+ int count = 0;
+ while ((partOfIp = strtok_r(partOfIp == NULL ? input : NULL, ".\0",
&savePtr)) != NULL) {
+ if (count > 3) {
+ celix_err_pushf("Failed to convert IP address %s to unsigned int,
to many parts", ip);
+ return 0;
+ }
+ bool longConverted = false;
+ long partAsLong = celix_utils_convertStringToLong(partOfIp, ULONG_MAX,
&longConverted);
+ if (!longConverted) {
+ celix_err_pushf("Failed to convert IP address %s to unsigned int,
part `%s` is not a number", ip, partOfIp);
+ return 0;
+ } else if (partAsLong > 255 || partAsLong < 0) {
+ celix_err_pushf("Failed to convert IP address %s to unsigned int,
part `%s` is out of range", ip, partOfIp);
+ return 0;
+ }
+ ipAsUint += (uint32_t)(partAsLong * powl(256, 3 - count++));
+ }
+
+ if (converted) {
+ *converted = true;
+ }
return ipAsUint;
}
-char *ipUtils_unsignedIntToIp(unsigned int ip) {
- char *ipStr = calloc(16, sizeof(char));
+char* celix_utils_convertUintToIp(uint32_t ip) {
+ char* ipStr = calloc(16, sizeof(char));
+ if (!ipStr) {
+ celix_err_push("Failed to allocate memory for IP address string");
+ return NULL;
+ }
- int ipPart1 = ip / (int) pow(256, 3); ip -= ipPart1 * (int) pow(256, 3);
- int ipPart2 = ip / (int) pow(256, 2); ip -= ipPart2 * (int) pow(256, 2);
- int ipPart3 = ip / (int) pow(256, 1); ip -= ipPart3 * (int) pow(256, 1);
- int ipPart4 = ip / (int) pow(256, 0);
+ int64_t ipPart1 = ip / (int64_t)pow(256, 3);
+ ip -= ipPart1 * (int64_t)pow(256, 3);
+ int64_t ipPart2 = ip / (int64_t)pow(256, 2);
+ ip -= ipPart2 * (int64_t)pow(256, 2);
+ int64_t ipPart3 = ip / (int64_t)pow(256, 1);
+ ip -= ipPart3 * (int64_t)pow(256, 1);
+ int64_t ipPart4 = ip / (int64_t)pow(256, 0);
- snprintf(ipStr, 16, "%d.%d.%d.%d", ipPart1, ipPart2, ipPart3, ipPart4);
+ snprintf(ipStr, 16, "%i.%i.%i.%i", (int)ipPart1, (int)ipPart2,
(int)ipPart3, (int)ipPart4);
return ipStr;
}
-unsigned int ipUtils_prefixToBitmask(unsigned int prefix) {
- return (0xFFFFFFFF << (32 - prefix)) & 0xFFFFFFFF;
+uint32_t celix_utils_ipPrefixLengthToBitmask(int prefix) {
+ if (prefix > 32 || prefix <= 0) {
+ return 0;
+ }
+ return (uint32_t )((0xFFFFFFFF << (32 - prefix)) & 0xFFFFFFFF);
}
-int ipUtils_netmaskToPrefix(const char *netmask) {
+int celix_utils_ipNetmaskToPrefixLength(const char* netmask) {
// Convert netmask to in_addr object
struct in_addr in;
int ret = inet_pton(AF_INET, netmask, &in);
if (ret != 1) {
+ celix_err_pushf("Failed to convert netmask %s to in_addr object",
netmask);
return -1;
}
// Now convert the mask to a prefix
int prefix = 0;
- bool processed_one = false;
- unsigned int i = ntohl(in.s_addr);
+ uint32_t i = ntohl(in.s_addr);
while (i > 0) {
if (i & 1) {
prefix++;
- processed_one = true;
- } else {
- if (processed_one) return -1;
}
-
i >>= 1;
}
return prefix;
}
-/** Finds an IP of the available network interfaces of the machine by
specifying an CIDR subnet.
- *
- * @param ipWithPrefix IP with prefix, e.g. 192.168.1.0/24
- * @return ip In case a matching interface could be found, an
allocated string containing the IP of the
- * interface will be returned, e.g. 192.168.1.16. Memory
for the new string can be freed with free().
- * When no matching interface is found NULL will be
returned.
- */
-char *ipUtils_findIpBySubnet(const char *ipWithPrefix) {
- char *ip = NULL;
+celix_status_t celix_utils_findIpInSubnet(const char* subnetCidrNotation,
char** foundIp) {
+ assert(subnetCidrNotation);
+ assert(foundIp);
+
+ *foundIp = NULL;
- char *input = celix_utils_strdup(ipWithPrefix); // Make a copy as
otherwise strtok_r manipulates the input string
- if (input == NULL) {
- goto strdup_failed;
+ //create copy for strtok_r
+ celix_autofree char* input = celix_utils_strdup(subnetCidrNotation);
Review Comment:
Considering that valid CIDR notation cannot be very long, `snprintf` into a
stack buffer and checking its return value should be enough. Memory allocation
and error injection efforts can be avoided.
##########
bundles/remote_services/rsa_spi/include/remote_service_admin.h:
##########
@@ -41,10 +41,10 @@ typedef struct remote_service_admin remote_service_admin_t;
struct remote_service_admin_service {
remote_service_admin_t *admin;
- celix_status_t (*exportService)(remote_service_admin_t *admin, char
*serviceId, celix_properties_t *properties, array_list_pt *registrations);
+ celix_status_t (*exportService)(remote_service_admin_t *admin, char
*serviceId, celix_properties_t *properties, celix_array_list_t** registrations);
celix_status_t (*removeExportedService)(remote_service_admin_t *admin,
export_registration_t *registration);
- celix_status_t (*getExportedServices)(remote_service_admin_t *admin,
array_list_pt *services);
- celix_status_t (*getImportedEndpoints)(remote_service_admin_t *admin,
array_list_pt *services);
+ celix_status_t (*getExportedServices)(remote_service_admin_t *admin,
celix_array_list_t** services);
Review Comment:
Missing #include "celix_array_list.h". It depends on transitive inclusion.
According to Large Scale C++ by Lakos, "transitive includes are not permitted,
except to access a public base class of a type whose definition is included
directly." We should include it because we need it directly, and thus should
not rely on other header file.
This remark applies to multiple places of this PR.
##########
bundles/remote_services/discovery_common/src/endpoint_discovery_poller.c:
##########
@@ -308,7 +316,7 @@ static void
*endpointDiscoveryPoller_performPeriodicPoll(void *data) {
hash_map_entry_pt entry =
hashMapIterator_nextEntry(iterator);
char *url = hashMapEntry_getKey(entry);
- array_list_pt currentEndpoints =
hashMapEntry_getValue(entry);
+ celix_array_list_t* currentEndpoints =
hashMapEntry_getValue(entry);
Review Comment:
nitpick, indentation.
##########
bundles/remote_services/topology_manager/src/scope.c:
##########
@@ -120,10 +120,13 @@ celix_status_t tm_addImportScope(void *handle, char
*filter) {
return CELIX_ILLEGAL_ARGUMENT; // filter not parsable
}
if (celixThreadMutex_lock(&scope->importScopeLock) == CELIX_SUCCESS) {
- int index = arrayList_indexOf(scope->importScopes, new);
- filter_pt present = (filter_pt) arrayList_get(scope->importScopes,
index);
+ celix_array_list_entry_t entry;
+ memset(&entry, 0, sizeof(entry));
+ entry.voidPtrVal = new;
+ int index = celix_arrayList_indexOf(scope->importScopes, entry);
Review Comment:
I found this dubious. If the intention is to add the filter to
`importScopes` if it is not there, then calling `celix_arrayList_get` on
`index` that may be -1 is simply illegal, i.e., out of bound memory access.
Please have a look at this, @xuzhenbao
##########
bundles/remote_services/discovery_common/src/endpoint_discovery_poller.c:
##########
@@ -183,111 +184,118 @@ celix_status_t
endpointDiscoveryPoller_getDiscoveryEndpoints(endpoint_discovery_
/**
* Adds a new endpoint URL to the list of polled endpoints.
*/
-celix_status_t
endpointDiscoveryPoller_addDiscoveryEndpoint(endpoint_discovery_poller_t
*poller, char *url) {
- celix_status_t status;
+celix_status_t
endpointDiscoveryPoller_addDiscoveryEndpoint(endpoint_discovery_poller_t*
poller, char* url) {
+ celix_status_t status;
- status = celixThreadMutex_lock(&(poller)->pollerLock);
- if (status != CELIX_SUCCESS) {
- return CELIX_BUNDLE_EXCEPTION;
- }
+ status = celixThreadMutex_lock(&(poller)->pollerLock);
+ if (status != CELIX_SUCCESS) {
+ return CELIX_BUNDLE_EXCEPTION;
+ }
- // Avoid memory leaks when adding an already existing URL...
- array_list_pt endpoints = hashMap_get(poller->entries, url);
- if (endpoints == NULL) {
- status =
arrayList_createWithEquals(endpointDiscoveryPoller_endpointDescriptionEquals,
&endpoints);
+ // Avoid memory leaks when adding an already existing URL...
+ celix_array_list_t* endpoints = hashMap_get(poller->entries, url);
+ if (endpoints == NULL) {
+ endpoints =
celix_arrayList_createWithEquals(endpointDiscoveryPoller_endpointDescriptionEquals);
- if (status == CELIX_SUCCESS) {
+ if (endpoints) {
celix_logHelper_debug(*poller->loghelper, "ENDPOINT_POLLER: add
new discovery endpoint with url %s", url);
- hashMap_put(poller->entries, strdup(url), endpoints);
- endpointDiscoveryPoller_poll(poller, url, endpoints);
- }
- }
+ hashMap_put(poller->entries, strdup(url), endpoints);
+ endpointDiscoveryPoller_poll(poller, url, endpoints);
+ }
+ }
- status = celixThreadMutex_unlock(&poller->pollerLock);
+ status = celixThreadMutex_unlock(&poller->pollerLock);
- return status;
+ return status;
}
/**
* Removes an endpoint URL from the list of polled endpoints.
*/
-celix_status_t
endpointDiscoveryPoller_removeDiscoveryEndpoint(endpoint_discovery_poller_t
*poller, char *url) {
- celix_status_t status = CELIX_SUCCESS;
-
- if (celixThreadMutex_lock(&poller->pollerLock) != CELIX_SUCCESS) {
- status = CELIX_BUNDLE_EXCEPTION;
- } else {
- hash_map_entry_pt entry = hashMap_getEntry(poller->entries,
url);
-
- if (entry == NULL) {
- celix_logHelper_debug(*poller->loghelper, "ENDPOINT_POLLER: There
was no entry found belonging to url %s - maybe already removed?", url);
- } else {
- char* origKey = hashMapEntry_getKey(entry);
+celix_status_t
endpointDiscoveryPoller_removeDiscoveryEndpoint(endpoint_discovery_poller_t*
poller, char* url) {
+ celix_status_t status = CELIX_SUCCESS;
+
+ if (celixThreadMutex_lock(&poller->pollerLock) != CELIX_SUCCESS) {
+ status = CELIX_BUNDLE_EXCEPTION;
+ } else {
+ hash_map_entry_pt entry = hashMap_getEntry(poller->entries, url);
+
+ if (entry == NULL) {
+ celix_logHelper_debug(
+ *poller->loghelper,
+ "ENDPOINT_POLLER: There was no entry found belonging to url %s
- maybe already removed?",
+ url);
+ } else {
+ char* origKey = hashMapEntry_getKey(entry);
celix_logHelper_debug(*poller->loghelper, "ENDPOINT_POLLER: remove
discovery endpoint with url %s", url);
- array_list_pt entries = hashMap_remove(poller->entries,
url);
+ celix_array_list_t* entries = hashMap_remove(poller->entries, url);
- if (entries != NULL) {
- for (unsigned int i = arrayList_size(entries);
i > 0; i--) {
- endpoint_description_t *endpoint =
arrayList_get(entries, i - 1);
-
discovery_removeDiscoveredEndpoint(poller->discovery, endpoint);
- arrayList_remove(entries, i - 1);
- endpointDescription_destroy(endpoint);
- }
- arrayList_destroy(entries);
- }
+ if (entries != NULL) {
+ for (unsigned int i = celix_arrayList_size(entries); i > 0;
i--) {
+ endpoint_description_t* endpoint =
celix_arrayList_get(entries, i - 1);
+ discovery_removeDiscoveredEndpoint(poller->discovery,
endpoint);
+ celix_arrayList_removeAt(entries, i - 1);
+ endpointDescription_destroy(endpoint);
+ }
+ celix_arrayList_destroy(entries);
+ }
- free(origKey);
- }
- status = celixThreadMutex_unlock(&poller->pollerLock);
- }
+ free(origKey);
+ }
+ status = celixThreadMutex_unlock(&poller->pollerLock);
+ }
- return status;
+ return status;
}
-
-
-
-celix_status_t endpointDiscoveryPoller_poll(endpoint_discovery_poller_t
*poller, char *url, array_list_pt currentEndpoints) {
- celix_status_t status;
- array_list_pt updatedEndpoints = NULL;
-
- // create an arraylist with a custom equality test to ensure we can
find endpoints properly...
-
arrayList_createWithEquals(endpointDiscoveryPoller_endpointDescriptionEquals,
&updatedEndpoints);
- status = endpointDiscoveryPoller_getEndpoints(poller, url,
&updatedEndpoints);
-
- if (status == CELIX_SUCCESS) {
- if (updatedEndpoints != NULL) {
- for (unsigned int i = arrayList_size(currentEndpoints);
i > 0; i--) {
- endpoint_description_t *endpoint =
arrayList_get(currentEndpoints, i - 1);
-
- if (!arrayList_contains(updatedEndpoints,
endpoint)) {
- status =
discovery_removeDiscoveredEndpoint(poller->discovery, endpoint);
- arrayList_remove(currentEndpoints, i -
1);
- endpointDescription_destroy(endpoint);
- }
- }
-
- for (int i = arrayList_size(updatedEndpoints); i > 0;
i--) {
- endpoint_description_t *endpoint =
arrayList_remove(updatedEndpoints, 0);
-
- if (!arrayList_contains(currentEndpoints,
endpoint)) {
- arrayList_add(currentEndpoints,
endpoint);
- status =
discovery_addDiscoveredEndpoint(poller->discovery, endpoint);
- } else {
- endpointDescription_destroy(endpoint);
-
- }
- }
- }
- }
-
- if (updatedEndpoints != NULL) {
- arrayList_destroy(updatedEndpoints);
- }
-
- return status;
+celix_status_t
+endpointDiscoveryPoller_poll(endpoint_discovery_poller_t* poller, char* url,
celix_array_list_t* currentEndpoints) {
+ celix_status_t status;
+ celix_array_list_t* updatedEndpoints = NULL;
+
+ // create an arraylist with a custom equality test to ensure we can find
endpoints properly...
+ updatedEndpoints =
celix_arrayList_createWithEquals(endpointDiscoveryPoller_endpointDescriptionEquals);
+ status = endpointDiscoveryPoller_getEndpoints(poller, url,
&updatedEndpoints);
+
+ if (updatedEndpoints && status == CELIX_SUCCESS) {
+ if (updatedEndpoints != NULL) {
+ for (int i = celix_arrayList_size(currentEndpoints); i > 0; i--) {
+ endpoint_description_t* endpoint =
celix_arrayList_get(currentEndpoints, i - 1);
+
+ celix_array_list_entry_t entry;
+ memset(&entry, 0, sizeof(entry));
+ entry.voidPtrVal = endpoint;
+
+ if (celix_arrayList_indexOf(updatedEndpoints, entry) < 0) {
+ status =
discovery_removeDiscoveredEndpoint(poller->discovery, endpoint);
+ celix_arrayList_removeAt(currentEndpoints, i - 1);
+ endpointDescription_destroy(endpoint);
+ }
+ }
+
+ for (int i = 0; i < celix_arrayList_size(updatedEndpoints); i++) {
+ endpoint_description_t* endpoint =
celix_arrayList_get(updatedEndpoints, i);
+ celix_array_list_entry_t entry;
+ memset(&entry, 0, sizeof(entry));
+ entry.voidPtrVal = endpoint;
+ if (celix_arrayList_indexOf(currentEndpoints, entry) < 0) {
+ celix_arrayList_add(currentEndpoints, endpoint);
+ status =
discovery_addDiscoveredEndpoint(poller->discovery, endpoint);
Review Comment:
What if `celix_arrayList_add` or `discovery_addDiscoveredEndpoint` fails?
Fixing this is out the scope of this PR, and could be left for further PR.
Maybe we need an issue similar to #686 for RSA.
##########
bundles/remote_services/topology_manager/src/scope.c:
##########
@@ -150,12 +153,15 @@ celix_status_t tm_removeImportScope(void *handle, char
*filter) {
}
if (celixThreadMutex_lock(&scope->importScopeLock) == CELIX_SUCCESS) {
- int index = arrayList_indexOf(scope->importScopes, new);
- filter_pt present = (filter_pt) arrayList_get(scope->importScopes,
index);
+ celix_array_list_entry_t entry;
+ memset(&entry, 0, sizeof(entry));
+ entry.voidPtrVal = new;
+ int index = celix_arrayList_indexOf(scope->importScopes, entry);
+ filter_pt present = (filter_pt)
celix_arrayList_get(scope->importScopes, index);
if (present == NULL)
Review Comment:
Could `present` be NULL if `index != -1`?
##########
bundles/remote_services/topology_manager/tms_tst/disc_mock/disc_mock_activator.c:
##########
@@ -44,12 +44,12 @@ celix_status_t
celix_bundleActivator_create(celix_bundle_context_t *context, voi
discMockService_create(act, &act->serv);
act->endpointListener = NULL;
act->endpointListenerService = NULL;
- status = arrayList_create(&act->endpointList);
+ act->endpointList = celix_arrayList_create();
} else {
status = CELIX_ENOMEM;
}
- if (status == CELIX_SUCCESS) {
+ if (status == CELIX_SUCCESS && act->endpointList) {
Review Comment:
Even if `act->endpointList == NULL`, `CELIX_SUCCESS` will be returned.
##########
bundles/shell/remote_shell/src/remote_shell.c:
##########
@@ -87,7 +92,7 @@ celix_status_t remoteShell_destroy(remote_shell_pt instance) {
remoteShell_stopConnections(instance);
celixThreadMutex_lock(&instance->mutex);
- arrayList_destroy(instance->connections);
+ celix_arrayList_destroy(instance->connections);
Review Comment:
nitpick, indentation.
##########
bundles/remote_services/discovery_common/src/endpoint_discovery_poller.c:
##########
@@ -183,111 +184,118 @@ celix_status_t
endpointDiscoveryPoller_getDiscoveryEndpoints(endpoint_discovery_
/**
* Adds a new endpoint URL to the list of polled endpoints.
*/
-celix_status_t
endpointDiscoveryPoller_addDiscoveryEndpoint(endpoint_discovery_poller_t
*poller, char *url) {
- celix_status_t status;
+celix_status_t
endpointDiscoveryPoller_addDiscoveryEndpoint(endpoint_discovery_poller_t*
poller, char* url) {
+ celix_status_t status;
- status = celixThreadMutex_lock(&(poller)->pollerLock);
- if (status != CELIX_SUCCESS) {
- return CELIX_BUNDLE_EXCEPTION;
- }
+ status = celixThreadMutex_lock(&(poller)->pollerLock);
+ if (status != CELIX_SUCCESS) {
+ return CELIX_BUNDLE_EXCEPTION;
+ }
- // Avoid memory leaks when adding an already existing URL...
- array_list_pt endpoints = hashMap_get(poller->entries, url);
- if (endpoints == NULL) {
- status =
arrayList_createWithEquals(endpointDiscoveryPoller_endpointDescriptionEquals,
&endpoints);
+ // Avoid memory leaks when adding an already existing URL...
+ celix_array_list_t* endpoints = hashMap_get(poller->entries, url);
+ if (endpoints == NULL) {
+ endpoints =
celix_arrayList_createWithEquals(endpointDiscoveryPoller_endpointDescriptionEquals);
- if (status == CELIX_SUCCESS) {
+ if (endpoints) {
celix_logHelper_debug(*poller->loghelper, "ENDPOINT_POLLER: add
new discovery endpoint with url %s", url);
- hashMap_put(poller->entries, strdup(url), endpoints);
- endpointDiscoveryPoller_poll(poller, url, endpoints);
- }
- }
+ hashMap_put(poller->entries, strdup(url), endpoints);
+ endpointDiscoveryPoller_poll(poller, url, endpoints);
+ }
+ }
- status = celixThreadMutex_unlock(&poller->pollerLock);
+ status = celixThreadMutex_unlock(&poller->pollerLock);
- return status;
+ return status;
}
/**
* Removes an endpoint URL from the list of polled endpoints.
*/
-celix_status_t
endpointDiscoveryPoller_removeDiscoveryEndpoint(endpoint_discovery_poller_t
*poller, char *url) {
- celix_status_t status = CELIX_SUCCESS;
-
- if (celixThreadMutex_lock(&poller->pollerLock) != CELIX_SUCCESS) {
- status = CELIX_BUNDLE_EXCEPTION;
- } else {
- hash_map_entry_pt entry = hashMap_getEntry(poller->entries,
url);
-
- if (entry == NULL) {
- celix_logHelper_debug(*poller->loghelper, "ENDPOINT_POLLER: There
was no entry found belonging to url %s - maybe already removed?", url);
- } else {
- char* origKey = hashMapEntry_getKey(entry);
+celix_status_t
endpointDiscoveryPoller_removeDiscoveryEndpoint(endpoint_discovery_poller_t*
poller, char* url) {
+ celix_status_t status = CELIX_SUCCESS;
+
+ if (celixThreadMutex_lock(&poller->pollerLock) != CELIX_SUCCESS) {
+ status = CELIX_BUNDLE_EXCEPTION;
+ } else {
+ hash_map_entry_pt entry = hashMap_getEntry(poller->entries, url);
+
+ if (entry == NULL) {
+ celix_logHelper_debug(
+ *poller->loghelper,
+ "ENDPOINT_POLLER: There was no entry found belonging to url %s
- maybe already removed?",
+ url);
+ } else {
+ char* origKey = hashMapEntry_getKey(entry);
celix_logHelper_debug(*poller->loghelper, "ENDPOINT_POLLER: remove
discovery endpoint with url %s", url);
- array_list_pt entries = hashMap_remove(poller->entries,
url);
+ celix_array_list_t* entries = hashMap_remove(poller->entries, url);
- if (entries != NULL) {
- for (unsigned int i = arrayList_size(entries);
i > 0; i--) {
- endpoint_description_t *endpoint =
arrayList_get(entries, i - 1);
-
discovery_removeDiscoveredEndpoint(poller->discovery, endpoint);
- arrayList_remove(entries, i - 1);
- endpointDescription_destroy(endpoint);
- }
- arrayList_destroy(entries);
- }
+ if (entries != NULL) {
+ for (unsigned int i = celix_arrayList_size(entries); i > 0;
i--) {
+ endpoint_description_t* endpoint =
celix_arrayList_get(entries, i - 1);
+ discovery_removeDiscoveredEndpoint(poller->discovery,
endpoint);
+ celix_arrayList_removeAt(entries, i - 1);
+ endpointDescription_destroy(endpoint);
+ }
+ celix_arrayList_destroy(entries);
+ }
- free(origKey);
- }
- status = celixThreadMutex_unlock(&poller->pollerLock);
- }
+ free(origKey);
+ }
+ status = celixThreadMutex_unlock(&poller->pollerLock);
+ }
- return status;
+ return status;
}
-
-
-
-celix_status_t endpointDiscoveryPoller_poll(endpoint_discovery_poller_t
*poller, char *url, array_list_pt currentEndpoints) {
- celix_status_t status;
- array_list_pt updatedEndpoints = NULL;
-
- // create an arraylist with a custom equality test to ensure we can
find endpoints properly...
-
arrayList_createWithEquals(endpointDiscoveryPoller_endpointDescriptionEquals,
&updatedEndpoints);
- status = endpointDiscoveryPoller_getEndpoints(poller, url,
&updatedEndpoints);
-
- if (status == CELIX_SUCCESS) {
- if (updatedEndpoints != NULL) {
- for (unsigned int i = arrayList_size(currentEndpoints);
i > 0; i--) {
- endpoint_description_t *endpoint =
arrayList_get(currentEndpoints, i - 1);
-
- if (!arrayList_contains(updatedEndpoints,
endpoint)) {
- status =
discovery_removeDiscoveredEndpoint(poller->discovery, endpoint);
- arrayList_remove(currentEndpoints, i -
1);
- endpointDescription_destroy(endpoint);
- }
- }
-
- for (int i = arrayList_size(updatedEndpoints); i > 0;
i--) {
- endpoint_description_t *endpoint =
arrayList_remove(updatedEndpoints, 0);
-
- if (!arrayList_contains(currentEndpoints,
endpoint)) {
- arrayList_add(currentEndpoints,
endpoint);
- status =
discovery_addDiscoveredEndpoint(poller->discovery, endpoint);
- } else {
- endpointDescription_destroy(endpoint);
-
- }
- }
- }
- }
-
- if (updatedEndpoints != NULL) {
- arrayList_destroy(updatedEndpoints);
- }
-
- return status;
+celix_status_t
+endpointDiscoveryPoller_poll(endpoint_discovery_poller_t* poller, char* url,
celix_array_list_t* currentEndpoints) {
+ celix_status_t status;
+ celix_array_list_t* updatedEndpoints = NULL;
+
+ // create an arraylist with a custom equality test to ensure we can find
endpoints properly...
+ updatedEndpoints =
celix_arrayList_createWithEquals(endpointDiscoveryPoller_endpointDescriptionEquals);
+ status = endpointDiscoveryPoller_getEndpoints(poller, url,
&updatedEndpoints);
+
+ if (updatedEndpoints && status == CELIX_SUCCESS) {
+ if (updatedEndpoints != NULL) {
+ for (int i = celix_arrayList_size(currentEndpoints); i > 0; i--) {
+ endpoint_description_t* endpoint =
celix_arrayList_get(currentEndpoints, i - 1);
+
+ celix_array_list_entry_t entry;
+ memset(&entry, 0, sizeof(entry));
+ entry.voidPtrVal = endpoint;
+
+ if (celix_arrayList_indexOf(updatedEndpoints, entry) < 0) {
+ status =
discovery_removeDiscoveredEndpoint(poller->discovery, endpoint);
+ celix_arrayList_removeAt(currentEndpoints, i - 1);
+ endpointDescription_destroy(endpoint);
+ }
+ }
+
+ for (int i = 0; i < celix_arrayList_size(updatedEndpoints); i++) {
+ endpoint_description_t* endpoint =
celix_arrayList_get(updatedEndpoints, i);
+ celix_array_list_entry_t entry;
+ memset(&entry, 0, sizeof(entry));
+ entry.voidPtrVal = endpoint;
+ if (celix_arrayList_indexOf(currentEndpoints, entry) < 0) {
+ celix_arrayList_add(currentEndpoints, endpoint);
+ status =
discovery_addDiscoveredEndpoint(poller->discovery, endpoint);
+ } else {
+ endpointDescription_destroy(endpoint);
+ }
+ }
+ celix_arrayList_clear(updatedEndpoints);
Review Comment:
Is it unnecessary if we have `celix_arrayList_destroy`?
##########
libs/framework/src/dm_dependency_manager_impl.c:
##########
@@ -367,13 +368,13 @@ celix_status_t
dependencyManager_getInfo(celix_dependency_manager_t *manager, dm
celixThreadMutex_lock(&manager->mutex);
if (info != NULL) {
- arrayList_create(&info->components);
- size = arrayList_size(manager->components);
+ info->components = celix_arrayList_create();
+ size = celix_arrayList_size(manager->components);
for (i = 0; i < size; i += 1) {
- celix_dm_component_t *cmp =
arrayList_get(manager->components, i);
+ celix_dm_component_t *cmp =
celix_arrayList_get(manager->components, i);
cmpInfo = NULL;
component_getComponentInfo(cmp, &cmpInfo);
- arrayList_add(info->components, cmpInfo);
+ celix_arrayList_add(info->components, cmpInfo);
Review Comment:
nitpick, indentation.
##########
libs/framework/src/bundle_context_private.h:
##########
@@ -80,7 +80,7 @@ struct celix_bundle_context {
celix_bundle_t *bundle;
celix_thread_mutex_t mutex; //protects fields below (NOTE/FIXME also
used by bundle.c for listing service tracker usage)
- array_list_t *svcRegistrations; //serviceIds
+ celix_array_list_t *svcRegistrations; //serviceIds
Review Comment:
nitpick, indentation.
##########
bundles/shell/remote_shell/src/remote_shell.c:
##########
@@ -204,7 +209,7 @@ void *remoteShell_connection_run(void *data) {
celix_logHelper_log(*connection->parent->loghelper, CELIX_LOG_LEVEL_INFO,
"REMOTE_SHELL: Closing socket");
celixThreadMutex_lock(&connection->parent->mutex);
- arrayList_removeElement(connection->parent->connections, connection);
+ celix_arrayList_remove(connection->parent->connections, connection);
Review Comment:
nitpick, indentation.
##########
bundles/shell/remote_shell/src/remote_shell.c:
##########
@@ -61,24 +61,29 @@ static celix_status_t
remoteShell_connection_print(connection_pt connection, cha
static celix_status_t remoteShell_connection_execute(connection_pt connection,
char *command);
static void* remoteShell_connection_run(void *data);
-celix_status_t remoteShell_create(shell_mediator_pt mediator, int
maximumConnections, remote_shell_pt *instance) {
- celix_status_t status = CELIX_SUCCESS;
- (*instance) = calloc(1, sizeof(**instance));
- if ((*instance) != NULL) {
- (*instance)->mediator = mediator;
- (*instance)->maximumConnections = maximumConnections;
- (*instance)->connections = NULL;
- (*instance)->loghelper = &mediator->loghelper;
-
- status = celixThreadMutex_create(&(*instance)->mutex, NULL);
-
- if (status == CELIX_SUCCESS) {
- status = arrayList_create(&(*instance)->connections);
- }
- } else {
- status = CELIX_ENOMEM;
- }
- return status;
+celix_status_t remoteShell_create(shell_mediator_pt mediator, int
maximumConnections, remote_shell_pt* instance) {
+ celix_status_t status = CELIX_SUCCESS;
+ (*instance) = calloc(1, sizeof(**instance));
+ if ((*instance) != NULL) {
+ (*instance)->mediator = mediator;
+ (*instance)->maximumConnections = maximumConnections;
+ (*instance)->connections = NULL;
+ (*instance)->loghelper = &mediator->loghelper;
+
+ status = celixThreadMutex_create(&(*instance)->mutex, NULL);
+
+ if (status == CELIX_SUCCESS) {
+ (*instance)->connections = celix_arrayList_create();
+ if (!(*instance)->connections) {
Review Comment:
`mutex` is not destroyed. celix_cleanup and early return will be handy.
##########
bundles/shell/remote_shell/src/remote_shell.c:
##########
@@ -106,9 +111,9 @@ celix_status_t remoteShell_addConnection(remote_shell_pt
instance, int socket) {
celixThreadMutex_lock(&instance->mutex);
- if (arrayList_size(instance->connections) <
instance->maximumConnections) {
+ if (celix_arrayList_size(instance->connections) <
instance->maximumConnections) {
celix_thread_t connectionRunThread =
celix_thread_default;
- arrayList_add(instance->connections,
connection);
+ celix_arrayList_add(instance->connections,
connection);
Review Comment:
nitpick, indentation.
##########
libs/utils/src/ip_utils.c:
##########
@@ -16,155 +16,207 @@
* specific language governing permissions and limitations
* under the License.
*/
-/**
- * ip_utils.c
- *
- * \date Jun 24, 2019
- * \author <a href="mailto:dev@celix.apache.org";>Apache Celix Project
Team</a>
- * \copyright Apache License, Version 2.0
- */
-#include "ip_utils.h"
+#include "celix_ip_utils.h"
+
+#include "celix_err.h"
+#include "celix_stdlib_cleanup.h"
#include "celix_utils.h"
+#include "celix_convert_utils.h"
+#include <arpa/inet.h>
+#include <assert.h>
#include <errno.h>
+#include <ifaddrs.h>
+#include <limits.h>
+#include <math.h>
+#include <netdb.h>
+#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>
-#include <stdbool.h>
#include <string.h>
-#include <math.h>
-
-#include <arpa/inet.h>
#include <sys/socket.h>
-#include <netdb.h>
-#include <ifaddrs.h>
-#include <unistd.h>
-unsigned int ipUtils_ipToUnsignedInt(char *ip) {
- unsigned int ipAsUint = 0;
+uint32_t celix_utils_convertIpToUint(const char* ip, bool* converted) {
+ if (converted) {
+ *converted = false;
+ }
- char *partOfIp = NULL, *savePtr = NULL;
- char *input = strdup(ip); // Make a copy because otherwise strtok_r
manipulates the input string
- partOfIp = strtok_r(input, ".\0", &savePtr); ipAsUint += strtoul(partOfIp,
NULL, 10) * (unsigned int) pow(256, 3);
- partOfIp = strtok_r(NULL, ".\0", &savePtr); ipAsUint += strtoul(partOfIp,
NULL, 10) * (unsigned int) pow(256, 2);
- partOfIp = strtok_r(NULL, ".\0", &savePtr); ipAsUint += strtoul(partOfIp,
NULL, 10) * (unsigned int) pow(256, 1);
- partOfIp = strtok_r(NULL, ".\0", &savePtr); ipAsUint += strtoul(partOfIp,
NULL, 10) * (unsigned int) pow(256, 0);
- free(input);
+ // copy for strtok_r
+ celix_autofree char* input = celix_utils_strdup(ip);
+ if (!input) {
+ celix_err_push("Failed to duplicate input string for IP conversion");
+ return 0;
+ }
+ uint32_t ipAsUint = 0;
+ char* partOfIp = NULL;
+ char* savePtr = NULL;
+ int count = 0;
+ while ((partOfIp = strtok_r(partOfIp == NULL ? input : NULL, ".\0",
&savePtr)) != NULL) {
+ if (count > 3) {
+ celix_err_pushf("Failed to convert IP address %s to unsigned int,
to many parts", ip);
+ return 0;
+ }
+ bool longConverted = false;
+ long partAsLong = celix_utils_convertStringToLong(partOfIp, ULONG_MAX,
&longConverted);
+ if (!longConverted) {
+ celix_err_pushf("Failed to convert IP address %s to unsigned int,
part `%s` is not a number", ip, partOfIp);
+ return 0;
+ } else if (partAsLong > 255 || partAsLong < 0) {
+ celix_err_pushf("Failed to convert IP address %s to unsigned int,
part `%s` is out of range", ip, partOfIp);
+ return 0;
+ }
+ ipAsUint += (uint32_t)(partAsLong * powl(256, 3 - count++));
+ }
+
+ if (converted) {
+ *converted = true;
+ }
return ipAsUint;
}
-char *ipUtils_unsignedIntToIp(unsigned int ip) {
- char *ipStr = calloc(16, sizeof(char));
+char* celix_utils_convertUintToIp(uint32_t ip) {
+ char* ipStr = calloc(16, sizeof(char));
+ if (!ipStr) {
+ celix_err_push("Failed to allocate memory for IP address string");
+ return NULL;
+ }
- int ipPart1 = ip / (int) pow(256, 3); ip -= ipPart1 * (int) pow(256, 3);
- int ipPart2 = ip / (int) pow(256, 2); ip -= ipPart2 * (int) pow(256, 2);
- int ipPart3 = ip / (int) pow(256, 1); ip -= ipPart3 * (int) pow(256, 1);
- int ipPart4 = ip / (int) pow(256, 0);
+ int64_t ipPart1 = ip / (int64_t)pow(256, 3);
Review Comment:
Can this be replaced by `inet_ntop`?
##########
bundles/remote_services/discovery_common/src/endpoint_discovery_poller.c:
##########
@@ -183,111 +184,118 @@ celix_status_t
endpointDiscoveryPoller_getDiscoveryEndpoints(endpoint_discovery_
/**
* Adds a new endpoint URL to the list of polled endpoints.
*/
-celix_status_t
endpointDiscoveryPoller_addDiscoveryEndpoint(endpoint_discovery_poller_t
*poller, char *url) {
- celix_status_t status;
+celix_status_t
endpointDiscoveryPoller_addDiscoveryEndpoint(endpoint_discovery_poller_t*
poller, char* url) {
+ celix_status_t status;
- status = celixThreadMutex_lock(&(poller)->pollerLock);
- if (status != CELIX_SUCCESS) {
- return CELIX_BUNDLE_EXCEPTION;
- }
+ status = celixThreadMutex_lock(&(poller)->pollerLock);
+ if (status != CELIX_SUCCESS) {
+ return CELIX_BUNDLE_EXCEPTION;
+ }
- // Avoid memory leaks when adding an already existing URL...
- array_list_pt endpoints = hashMap_get(poller->entries, url);
- if (endpoints == NULL) {
- status =
arrayList_createWithEquals(endpointDiscoveryPoller_endpointDescriptionEquals,
&endpoints);
+ // Avoid memory leaks when adding an already existing URL...
+ celix_array_list_t* endpoints = hashMap_get(poller->entries, url);
+ if (endpoints == NULL) {
+ endpoints =
celix_arrayList_createWithEquals(endpointDiscoveryPoller_endpointDescriptionEquals);
- if (status == CELIX_SUCCESS) {
+ if (endpoints) {
celix_logHelper_debug(*poller->loghelper, "ENDPOINT_POLLER: add
new discovery endpoint with url %s", url);
- hashMap_put(poller->entries, strdup(url), endpoints);
- endpointDiscoveryPoller_poll(poller, url, endpoints);
- }
- }
+ hashMap_put(poller->entries, strdup(url), endpoints);
+ endpointDiscoveryPoller_poll(poller, url, endpoints);
+ }
+ }
- status = celixThreadMutex_unlock(&poller->pollerLock);
+ status = celixThreadMutex_unlock(&poller->pollerLock);
- return status;
+ return status;
}
/**
* Removes an endpoint URL from the list of polled endpoints.
*/
-celix_status_t
endpointDiscoveryPoller_removeDiscoveryEndpoint(endpoint_discovery_poller_t
*poller, char *url) {
- celix_status_t status = CELIX_SUCCESS;
-
- if (celixThreadMutex_lock(&poller->pollerLock) != CELIX_SUCCESS) {
- status = CELIX_BUNDLE_EXCEPTION;
- } else {
- hash_map_entry_pt entry = hashMap_getEntry(poller->entries,
url);
-
- if (entry == NULL) {
- celix_logHelper_debug(*poller->loghelper, "ENDPOINT_POLLER: There
was no entry found belonging to url %s - maybe already removed?", url);
- } else {
- char* origKey = hashMapEntry_getKey(entry);
+celix_status_t
endpointDiscoveryPoller_removeDiscoveryEndpoint(endpoint_discovery_poller_t*
poller, char* url) {
+ celix_status_t status = CELIX_SUCCESS;
+
+ if (celixThreadMutex_lock(&poller->pollerLock) != CELIX_SUCCESS) {
+ status = CELIX_BUNDLE_EXCEPTION;
+ } else {
+ hash_map_entry_pt entry = hashMap_getEntry(poller->entries, url);
+
+ if (entry == NULL) {
+ celix_logHelper_debug(
+ *poller->loghelper,
+ "ENDPOINT_POLLER: There was no entry found belonging to url %s
- maybe already removed?",
+ url);
+ } else {
+ char* origKey = hashMapEntry_getKey(entry);
celix_logHelper_debug(*poller->loghelper, "ENDPOINT_POLLER: remove
discovery endpoint with url %s", url);
- array_list_pt entries = hashMap_remove(poller->entries,
url);
+ celix_array_list_t* entries = hashMap_remove(poller->entries, url);
- if (entries != NULL) {
- for (unsigned int i = arrayList_size(entries);
i > 0; i--) {
- endpoint_description_t *endpoint =
arrayList_get(entries, i - 1);
-
discovery_removeDiscoveredEndpoint(poller->discovery, endpoint);
- arrayList_remove(entries, i - 1);
- endpointDescription_destroy(endpoint);
- }
- arrayList_destroy(entries);
- }
+ if (entries != NULL) {
+ for (unsigned int i = celix_arrayList_size(entries); i > 0;
i--) {
+ endpoint_description_t* endpoint =
celix_arrayList_get(entries, i - 1);
+ discovery_removeDiscoveredEndpoint(poller->discovery,
endpoint);
+ celix_arrayList_removeAt(entries, i - 1);
+ endpointDescription_destroy(endpoint);
+ }
+ celix_arrayList_destroy(entries);
+ }
- free(origKey);
- }
- status = celixThreadMutex_unlock(&poller->pollerLock);
- }
+ free(origKey);
+ }
+ status = celixThreadMutex_unlock(&poller->pollerLock);
+ }
- return status;
+ return status;
}
-
-
-
-celix_status_t endpointDiscoveryPoller_poll(endpoint_discovery_poller_t
*poller, char *url, array_list_pt currentEndpoints) {
- celix_status_t status;
- array_list_pt updatedEndpoints = NULL;
-
- // create an arraylist with a custom equality test to ensure we can
find endpoints properly...
-
arrayList_createWithEquals(endpointDiscoveryPoller_endpointDescriptionEquals,
&updatedEndpoints);
- status = endpointDiscoveryPoller_getEndpoints(poller, url,
&updatedEndpoints);
-
- if (status == CELIX_SUCCESS) {
- if (updatedEndpoints != NULL) {
- for (unsigned int i = arrayList_size(currentEndpoints);
i > 0; i--) {
- endpoint_description_t *endpoint =
arrayList_get(currentEndpoints, i - 1);
-
- if (!arrayList_contains(updatedEndpoints,
endpoint)) {
- status =
discovery_removeDiscoveredEndpoint(poller->discovery, endpoint);
- arrayList_remove(currentEndpoints, i -
1);
- endpointDescription_destroy(endpoint);
- }
- }
-
- for (int i = arrayList_size(updatedEndpoints); i > 0;
i--) {
- endpoint_description_t *endpoint =
arrayList_remove(updatedEndpoints, 0);
-
- if (!arrayList_contains(currentEndpoints,
endpoint)) {
- arrayList_add(currentEndpoints,
endpoint);
- status =
discovery_addDiscoveredEndpoint(poller->discovery, endpoint);
- } else {
- endpointDescription_destroy(endpoint);
-
- }
- }
- }
- }
-
- if (updatedEndpoints != NULL) {
- arrayList_destroy(updatedEndpoints);
- }
-
- return status;
+celix_status_t
+endpointDiscoveryPoller_poll(endpoint_discovery_poller_t* poller, char* url,
celix_array_list_t* currentEndpoints) {
+ celix_status_t status;
+ celix_array_list_t* updatedEndpoints = NULL;
+
+ // create an arraylist with a custom equality test to ensure we can find
endpoints properly...
+ updatedEndpoints =
celix_arrayList_createWithEquals(endpointDiscoveryPoller_endpointDescriptionEquals);
+ status = endpointDiscoveryPoller_getEndpoints(poller, url,
&updatedEndpoints);
+
+ if (updatedEndpoints && status == CELIX_SUCCESS) {
+ if (updatedEndpoints != NULL) {
+ for (int i = celix_arrayList_size(currentEndpoints); i > 0; i--) {
+ endpoint_description_t* endpoint =
celix_arrayList_get(currentEndpoints, i - 1);
+
+ celix_array_list_entry_t entry;
+ memset(&entry, 0, sizeof(entry));
Review Comment:
`celix_array_list_entry_t entry = {0};` may be better than `memset`?
##########
libs/utils/src/ip_utils.c:
##########
@@ -16,155 +16,207 @@
* specific language governing permissions and limitations
* under the License.
*/
-/**
- * ip_utils.c
- *
- * \date Jun 24, 2019
- * \author <a href="mailto:dev@celix.apache.org";>Apache Celix Project
Team</a>
- * \copyright Apache License, Version 2.0
- */
-#include "ip_utils.h"
+#include "celix_ip_utils.h"
+
+#include "celix_err.h"
+#include "celix_stdlib_cleanup.h"
#include "celix_utils.h"
+#include "celix_convert_utils.h"
+#include <arpa/inet.h>
+#include <assert.h>
#include <errno.h>
+#include <ifaddrs.h>
+#include <limits.h>
+#include <math.h>
+#include <netdb.h>
+#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>
-#include <stdbool.h>
#include <string.h>
-#include <math.h>
-
-#include <arpa/inet.h>
#include <sys/socket.h>
-#include <netdb.h>
-#include <ifaddrs.h>
-#include <unistd.h>
-unsigned int ipUtils_ipToUnsignedInt(char *ip) {
- unsigned int ipAsUint = 0;
+uint32_t celix_utils_convertIpToUint(const char* ip, bool* converted) {
+ if (converted) {
+ *converted = false;
+ }
- char *partOfIp = NULL, *savePtr = NULL;
- char *input = strdup(ip); // Make a copy because otherwise strtok_r
manipulates the input string
- partOfIp = strtok_r(input, ".\0", &savePtr); ipAsUint += strtoul(partOfIp,
NULL, 10) * (unsigned int) pow(256, 3);
- partOfIp = strtok_r(NULL, ".\0", &savePtr); ipAsUint += strtoul(partOfIp,
NULL, 10) * (unsigned int) pow(256, 2);
- partOfIp = strtok_r(NULL, ".\0", &savePtr); ipAsUint += strtoul(partOfIp,
NULL, 10) * (unsigned int) pow(256, 1);
- partOfIp = strtok_r(NULL, ".\0", &savePtr); ipAsUint += strtoul(partOfIp,
NULL, 10) * (unsigned int) pow(256, 0);
- free(input);
+ // copy for strtok_r
+ celix_autofree char* input = celix_utils_strdup(ip);
+ if (!input) {
+ celix_err_push("Failed to duplicate input string for IP conversion");
+ return 0;
+ }
+ uint32_t ipAsUint = 0;
+ char* partOfIp = NULL;
+ char* savePtr = NULL;
+ int count = 0;
+ while ((partOfIp = strtok_r(partOfIp == NULL ? input : NULL, ".\0",
&savePtr)) != NULL) {
+ if (count > 3) {
+ celix_err_pushf("Failed to convert IP address %s to unsigned int,
to many parts", ip);
+ return 0;
+ }
+ bool longConverted = false;
+ long partAsLong = celix_utils_convertStringToLong(partOfIp, ULONG_MAX,
&longConverted);
+ if (!longConverted) {
+ celix_err_pushf("Failed to convert IP address %s to unsigned int,
part `%s` is not a number", ip, partOfIp);
+ return 0;
+ } else if (partAsLong > 255 || partAsLong < 0) {
+ celix_err_pushf("Failed to convert IP address %s to unsigned int,
part `%s` is out of range", ip, partOfIp);
+ return 0;
+ }
+ ipAsUint += (uint32_t)(partAsLong * powl(256, 3 - count++));
+ }
+
+ if (converted) {
+ *converted = true;
+ }
return ipAsUint;
}
-char *ipUtils_unsignedIntToIp(unsigned int ip) {
- char *ipStr = calloc(16, sizeof(char));
+char* celix_utils_convertUintToIp(uint32_t ip) {
+ char* ipStr = calloc(16, sizeof(char));
+ if (!ipStr) {
+ celix_err_push("Failed to allocate memory for IP address string");
+ return NULL;
+ }
- int ipPart1 = ip / (int) pow(256, 3); ip -= ipPart1 * (int) pow(256, 3);
- int ipPart2 = ip / (int) pow(256, 2); ip -= ipPart2 * (int) pow(256, 2);
- int ipPart3 = ip / (int) pow(256, 1); ip -= ipPart3 * (int) pow(256, 1);
- int ipPart4 = ip / (int) pow(256, 0);
+ int64_t ipPart1 = ip / (int64_t)pow(256, 3);
+ ip -= ipPart1 * (int64_t)pow(256, 3);
+ int64_t ipPart2 = ip / (int64_t)pow(256, 2);
+ ip -= ipPart2 * (int64_t)pow(256, 2);
+ int64_t ipPart3 = ip / (int64_t)pow(256, 1);
+ ip -= ipPart3 * (int64_t)pow(256, 1);
+ int64_t ipPart4 = ip / (int64_t)pow(256, 0);
- snprintf(ipStr, 16, "%d.%d.%d.%d", ipPart1, ipPart2, ipPart3, ipPart4);
+ snprintf(ipStr, 16, "%i.%i.%i.%i", (int)ipPart1, (int)ipPart2,
(int)ipPart3, (int)ipPart4);
return ipStr;
}
-unsigned int ipUtils_prefixToBitmask(unsigned int prefix) {
- return (0xFFFFFFFF << (32 - prefix)) & 0xFFFFFFFF;
+uint32_t celix_utils_ipPrefixLengthToBitmask(int prefix) {
+ if (prefix > 32 || prefix <= 0) {
+ return 0;
Review Comment:
Will 0xFFFFFFFF make more sense for prefix > 32?
##########
libs/utils/src/ip_utils.c:
##########
@@ -16,155 +16,207 @@
* specific language governing permissions and limitations
* under the License.
*/
-/**
- * ip_utils.c
- *
- * \date Jun 24, 2019
- * \author <a href="mailto:dev@celix.apache.org";>Apache Celix Project
Team</a>
- * \copyright Apache License, Version 2.0
- */
-#include "ip_utils.h"
+#include "celix_ip_utils.h"
+
+#include "celix_err.h"
+#include "celix_stdlib_cleanup.h"
#include "celix_utils.h"
+#include "celix_convert_utils.h"
+#include <arpa/inet.h>
+#include <assert.h>
#include <errno.h>
+#include <ifaddrs.h>
+#include <limits.h>
+#include <math.h>
+#include <netdb.h>
+#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>
-#include <stdbool.h>
#include <string.h>
-#include <math.h>
-
-#include <arpa/inet.h>
#include <sys/socket.h>
-#include <netdb.h>
-#include <ifaddrs.h>
-#include <unistd.h>
-unsigned int ipUtils_ipToUnsignedInt(char *ip) {
- unsigned int ipAsUint = 0;
+uint32_t celix_utils_convertIpToUint(const char* ip, bool* converted) {
+ if (converted) {
+ *converted = false;
+ }
- char *partOfIp = NULL, *savePtr = NULL;
- char *input = strdup(ip); // Make a copy because otherwise strtok_r
manipulates the input string
- partOfIp = strtok_r(input, ".\0", &savePtr); ipAsUint += strtoul(partOfIp,
NULL, 10) * (unsigned int) pow(256, 3);
- partOfIp = strtok_r(NULL, ".\0", &savePtr); ipAsUint += strtoul(partOfIp,
NULL, 10) * (unsigned int) pow(256, 2);
- partOfIp = strtok_r(NULL, ".\0", &savePtr); ipAsUint += strtoul(partOfIp,
NULL, 10) * (unsigned int) pow(256, 1);
- partOfIp = strtok_r(NULL, ".\0", &savePtr); ipAsUint += strtoul(partOfIp,
NULL, 10) * (unsigned int) pow(256, 0);
- free(input);
+ // copy for strtok_r
+ celix_autofree char* input = celix_utils_strdup(ip);
+ if (!input) {
+ celix_err_push("Failed to duplicate input string for IP conversion");
+ return 0;
+ }
+ uint32_t ipAsUint = 0;
+ char* partOfIp = NULL;
+ char* savePtr = NULL;
+ int count = 0;
+ while ((partOfIp = strtok_r(partOfIp == NULL ? input : NULL, ".\0",
&savePtr)) != NULL) {
+ if (count > 3) {
+ celix_err_pushf("Failed to convert IP address %s to unsigned int,
to many parts", ip);
+ return 0;
+ }
+ bool longConverted = false;
+ long partAsLong = celix_utils_convertStringToLong(partOfIp, ULONG_MAX,
&longConverted);
+ if (!longConverted) {
+ celix_err_pushf("Failed to convert IP address %s to unsigned int,
part `%s` is not a number", ip, partOfIp);
+ return 0;
+ } else if (partAsLong > 255 || partAsLong < 0) {
+ celix_err_pushf("Failed to convert IP address %s to unsigned int,
part `%s` is out of range", ip, partOfIp);
+ return 0;
+ }
+ ipAsUint += (uint32_t)(partAsLong * powl(256, 3 - count++));
+ }
+
+ if (converted) {
+ *converted = true;
+ }
return ipAsUint;
}
-char *ipUtils_unsignedIntToIp(unsigned int ip) {
- char *ipStr = calloc(16, sizeof(char));
+char* celix_utils_convertUintToIp(uint32_t ip) {
+ char* ipStr = calloc(16, sizeof(char));
+ if (!ipStr) {
+ celix_err_push("Failed to allocate memory for IP address string");
+ return NULL;
+ }
- int ipPart1 = ip / (int) pow(256, 3); ip -= ipPart1 * (int) pow(256, 3);
- int ipPart2 = ip / (int) pow(256, 2); ip -= ipPart2 * (int) pow(256, 2);
- int ipPart3 = ip / (int) pow(256, 1); ip -= ipPart3 * (int) pow(256, 1);
- int ipPart4 = ip / (int) pow(256, 0);
+ int64_t ipPart1 = ip / (int64_t)pow(256, 3);
+ ip -= ipPart1 * (int64_t)pow(256, 3);
+ int64_t ipPart2 = ip / (int64_t)pow(256, 2);
+ ip -= ipPart2 * (int64_t)pow(256, 2);
+ int64_t ipPart3 = ip / (int64_t)pow(256, 1);
+ ip -= ipPart3 * (int64_t)pow(256, 1);
+ int64_t ipPart4 = ip / (int64_t)pow(256, 0);
- snprintf(ipStr, 16, "%d.%d.%d.%d", ipPart1, ipPart2, ipPart3, ipPart4);
+ snprintf(ipStr, 16, "%i.%i.%i.%i", (int)ipPart1, (int)ipPart2,
(int)ipPart3, (int)ipPart4);
return ipStr;
}
-unsigned int ipUtils_prefixToBitmask(unsigned int prefix) {
- return (0xFFFFFFFF << (32 - prefix)) & 0xFFFFFFFF;
+uint32_t celix_utils_ipPrefixLengthToBitmask(int prefix) {
+ if (prefix > 32 || prefix <= 0) {
+ return 0;
+ }
+ return (uint32_t )((0xFFFFFFFF << (32 - prefix)) & 0xFFFFFFFF);
}
-int ipUtils_netmaskToPrefix(const char *netmask) {
+int celix_utils_ipNetmaskToPrefixLength(const char* netmask) {
// Convert netmask to in_addr object
struct in_addr in;
int ret = inet_pton(AF_INET, netmask, &in);
Review Comment:
We need to verify that `netmask` is indeed a valid ipv4 network mask.
##########
libs/utils/src/ip_utils.c:
##########
@@ -16,155 +16,207 @@
* specific language governing permissions and limitations
* under the License.
*/
-/**
- * ip_utils.c
- *
- * \date Jun 24, 2019
- * \author <a href="mailto:dev@celix.apache.org";>Apache Celix Project
Team</a>
- * \copyright Apache License, Version 2.0
- */
-#include "ip_utils.h"
+#include "celix_ip_utils.h"
+
+#include "celix_err.h"
+#include "celix_stdlib_cleanup.h"
#include "celix_utils.h"
+#include "celix_convert_utils.h"
+#include <arpa/inet.h>
+#include <assert.h>
#include <errno.h>
+#include <ifaddrs.h>
+#include <limits.h>
+#include <math.h>
+#include <netdb.h>
+#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>
-#include <stdbool.h>
#include <string.h>
-#include <math.h>
-
-#include <arpa/inet.h>
#include <sys/socket.h>
-#include <netdb.h>
-#include <ifaddrs.h>
-#include <unistd.h>
-unsigned int ipUtils_ipToUnsignedInt(char *ip) {
- unsigned int ipAsUint = 0;
+uint32_t celix_utils_convertIpToUint(const char* ip, bool* converted) {
+ if (converted) {
+ *converted = false;
+ }
- char *partOfIp = NULL, *savePtr = NULL;
- char *input = strdup(ip); // Make a copy because otherwise strtok_r
manipulates the input string
- partOfIp = strtok_r(input, ".\0", &savePtr); ipAsUint += strtoul(partOfIp,
NULL, 10) * (unsigned int) pow(256, 3);
- partOfIp = strtok_r(NULL, ".\0", &savePtr); ipAsUint += strtoul(partOfIp,
NULL, 10) * (unsigned int) pow(256, 2);
- partOfIp = strtok_r(NULL, ".\0", &savePtr); ipAsUint += strtoul(partOfIp,
NULL, 10) * (unsigned int) pow(256, 1);
- partOfIp = strtok_r(NULL, ".\0", &savePtr); ipAsUint += strtoul(partOfIp,
NULL, 10) * (unsigned int) pow(256, 0);
- free(input);
+ // copy for strtok_r
+ celix_autofree char* input = celix_utils_strdup(ip);
+ if (!input) {
+ celix_err_push("Failed to duplicate input string for IP conversion");
+ return 0;
+ }
+ uint32_t ipAsUint = 0;
+ char* partOfIp = NULL;
+ char* savePtr = NULL;
+ int count = 0;
+ while ((partOfIp = strtok_r(partOfIp == NULL ? input : NULL, ".\0",
&savePtr)) != NULL) {
+ if (count > 3) {
+ celix_err_pushf("Failed to convert IP address %s to unsigned int,
to many parts", ip);
+ return 0;
+ }
+ bool longConverted = false;
+ long partAsLong = celix_utils_convertStringToLong(partOfIp, ULONG_MAX,
&longConverted);
+ if (!longConverted) {
+ celix_err_pushf("Failed to convert IP address %s to unsigned int,
part `%s` is not a number", ip, partOfIp);
+ return 0;
+ } else if (partAsLong > 255 || partAsLong < 0) {
+ celix_err_pushf("Failed to convert IP address %s to unsigned int,
part `%s` is out of range", ip, partOfIp);
+ return 0;
+ }
+ ipAsUint += (uint32_t)(partAsLong * powl(256, 3 - count++));
+ }
+
+ if (converted) {
+ *converted = true;
+ }
return ipAsUint;
}
-char *ipUtils_unsignedIntToIp(unsigned int ip) {
- char *ipStr = calloc(16, sizeof(char));
+char* celix_utils_convertUintToIp(uint32_t ip) {
+ char* ipStr = calloc(16, sizeof(char));
+ if (!ipStr) {
+ celix_err_push("Failed to allocate memory for IP address string");
+ return NULL;
+ }
- int ipPart1 = ip / (int) pow(256, 3); ip -= ipPart1 * (int) pow(256, 3);
- int ipPart2 = ip / (int) pow(256, 2); ip -= ipPart2 * (int) pow(256, 2);
- int ipPart3 = ip / (int) pow(256, 1); ip -= ipPart3 * (int) pow(256, 1);
- int ipPart4 = ip / (int) pow(256, 0);
+ int64_t ipPart1 = ip / (int64_t)pow(256, 3);
+ ip -= ipPart1 * (int64_t)pow(256, 3);
+ int64_t ipPart2 = ip / (int64_t)pow(256, 2);
+ ip -= ipPart2 * (int64_t)pow(256, 2);
+ int64_t ipPart3 = ip / (int64_t)pow(256, 1);
+ ip -= ipPart3 * (int64_t)pow(256, 1);
+ int64_t ipPart4 = ip / (int64_t)pow(256, 0);
- snprintf(ipStr, 16, "%d.%d.%d.%d", ipPart1, ipPart2, ipPart3, ipPart4);
+ snprintf(ipStr, 16, "%i.%i.%i.%i", (int)ipPart1, (int)ipPart2,
(int)ipPart3, (int)ipPart4);
return ipStr;
}
-unsigned int ipUtils_prefixToBitmask(unsigned int prefix) {
- return (0xFFFFFFFF << (32 - prefix)) & 0xFFFFFFFF;
+uint32_t celix_utils_ipPrefixLengthToBitmask(int prefix) {
+ if (prefix > 32 || prefix <= 0) {
+ return 0;
+ }
+ return (uint32_t )((0xFFFFFFFF << (32 - prefix)) & 0xFFFFFFFF);
}
-int ipUtils_netmaskToPrefix(const char *netmask) {
+int celix_utils_ipNetmaskToPrefixLength(const char* netmask) {
Review Comment:
It seems to me that `uint32_t netmask` makes more sense than `const char*
netmask`.
Moreover, for uint32_t, some extra bit operations are enough to verify it is
indeed a valid ipv4 network mask. For string form mask, it is not that easy to
do that verification.
##########
libs/utils/src/ip_utils.c:
##########
@@ -16,155 +16,207 @@
* specific language governing permissions and limitations
* under the License.
*/
-/**
- * ip_utils.c
- *
- * \date Jun 24, 2019
- * \author <a href="mailto:dev@celix.apache.org";>Apache Celix Project
Team</a>
- * \copyright Apache License, Version 2.0
- */
-#include "ip_utils.h"
+#include "celix_ip_utils.h"
+
+#include "celix_err.h"
+#include "celix_stdlib_cleanup.h"
#include "celix_utils.h"
+#include "celix_convert_utils.h"
+#include <arpa/inet.h>
+#include <assert.h>
#include <errno.h>
+#include <ifaddrs.h>
+#include <limits.h>
+#include <math.h>
+#include <netdb.h>
+#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>
-#include <stdbool.h>
#include <string.h>
-#include <math.h>
-
-#include <arpa/inet.h>
#include <sys/socket.h>
-#include <netdb.h>
-#include <ifaddrs.h>
-#include <unistd.h>
-unsigned int ipUtils_ipToUnsignedInt(char *ip) {
- unsigned int ipAsUint = 0;
+uint32_t celix_utils_convertIpToUint(const char* ip, bool* converted) {
+ if (converted) {
+ *converted = false;
+ }
- char *partOfIp = NULL, *savePtr = NULL;
- char *input = strdup(ip); // Make a copy because otherwise strtok_r
manipulates the input string
- partOfIp = strtok_r(input, ".\0", &savePtr); ipAsUint += strtoul(partOfIp,
NULL, 10) * (unsigned int) pow(256, 3);
- partOfIp = strtok_r(NULL, ".\0", &savePtr); ipAsUint += strtoul(partOfIp,
NULL, 10) * (unsigned int) pow(256, 2);
- partOfIp = strtok_r(NULL, ".\0", &savePtr); ipAsUint += strtoul(partOfIp,
NULL, 10) * (unsigned int) pow(256, 1);
- partOfIp = strtok_r(NULL, ".\0", &savePtr); ipAsUint += strtoul(partOfIp,
NULL, 10) * (unsigned int) pow(256, 0);
- free(input);
+ // copy for strtok_r
+ celix_autofree char* input = celix_utils_strdup(ip);
+ if (!input) {
+ celix_err_push("Failed to duplicate input string for IP conversion");
+ return 0;
+ }
+ uint32_t ipAsUint = 0;
+ char* partOfIp = NULL;
+ char* savePtr = NULL;
+ int count = 0;
+ while ((partOfIp = strtok_r(partOfIp == NULL ? input : NULL, ".\0",
&savePtr)) != NULL) {
+ if (count > 3) {
+ celix_err_pushf("Failed to convert IP address %s to unsigned int,
to many parts", ip);
+ return 0;
+ }
+ bool longConverted = false;
+ long partAsLong = celix_utils_convertStringToLong(partOfIp, ULONG_MAX,
&longConverted);
+ if (!longConverted) {
+ celix_err_pushf("Failed to convert IP address %s to unsigned int,
part `%s` is not a number", ip, partOfIp);
+ return 0;
+ } else if (partAsLong > 255 || partAsLong < 0) {
+ celix_err_pushf("Failed to convert IP address %s to unsigned int,
part `%s` is out of range", ip, partOfIp);
+ return 0;
+ }
+ ipAsUint += (uint32_t)(partAsLong * powl(256, 3 - count++));
+ }
+
+ if (converted) {
+ *converted = true;
+ }
return ipAsUint;
}
-char *ipUtils_unsignedIntToIp(unsigned int ip) {
- char *ipStr = calloc(16, sizeof(char));
+char* celix_utils_convertUintToIp(uint32_t ip) {
+ char* ipStr = calloc(16, sizeof(char));
+ if (!ipStr) {
+ celix_err_push("Failed to allocate memory for IP address string");
+ return NULL;
+ }
- int ipPart1 = ip / (int) pow(256, 3); ip -= ipPart1 * (int) pow(256, 3);
- int ipPart2 = ip / (int) pow(256, 2); ip -= ipPart2 * (int) pow(256, 2);
- int ipPart3 = ip / (int) pow(256, 1); ip -= ipPart3 * (int) pow(256, 1);
- int ipPart4 = ip / (int) pow(256, 0);
+ int64_t ipPart1 = ip / (int64_t)pow(256, 3);
+ ip -= ipPart1 * (int64_t)pow(256, 3);
+ int64_t ipPart2 = ip / (int64_t)pow(256, 2);
+ ip -= ipPart2 * (int64_t)pow(256, 2);
+ int64_t ipPart3 = ip / (int64_t)pow(256, 1);
+ ip -= ipPart3 * (int64_t)pow(256, 1);
+ int64_t ipPart4 = ip / (int64_t)pow(256, 0);
- snprintf(ipStr, 16, "%d.%d.%d.%d", ipPart1, ipPart2, ipPart3, ipPart4);
+ snprintf(ipStr, 16, "%i.%i.%i.%i", (int)ipPart1, (int)ipPart2,
(int)ipPart3, (int)ipPart4);
return ipStr;
}
-unsigned int ipUtils_prefixToBitmask(unsigned int prefix) {
- return (0xFFFFFFFF << (32 - prefix)) & 0xFFFFFFFF;
+uint32_t celix_utils_ipPrefixLengthToBitmask(int prefix) {
+ if (prefix > 32 || prefix <= 0) {
+ return 0;
+ }
+ return (uint32_t )((0xFFFFFFFF << (32 - prefix)) & 0xFFFFFFFF);
}
-int ipUtils_netmaskToPrefix(const char *netmask) {
+int celix_utils_ipNetmaskToPrefixLength(const char* netmask) {
// Convert netmask to in_addr object
struct in_addr in;
int ret = inet_pton(AF_INET, netmask, &in);
if (ret != 1) {
+ celix_err_pushf("Failed to convert netmask %s to in_addr object",
netmask);
return -1;
}
// Now convert the mask to a prefix
int prefix = 0;
- bool processed_one = false;
- unsigned int i = ntohl(in.s_addr);
+ uint32_t i = ntohl(in.s_addr);
while (i > 0) {
if (i & 1) {
prefix++;
- processed_one = true;
- } else {
- if (processed_one) return -1;
}
-
i >>= 1;
}
return prefix;
}
-/** Finds an IP of the available network interfaces of the machine by
specifying an CIDR subnet.
- *
- * @param ipWithPrefix IP with prefix, e.g. 192.168.1.0/24
- * @return ip In case a matching interface could be found, an
allocated string containing the IP of the
- * interface will be returned, e.g. 192.168.1.16. Memory
for the new string can be freed with free().
- * When no matching interface is found NULL will be
returned.
- */
-char *ipUtils_findIpBySubnet(const char *ipWithPrefix) {
- char *ip = NULL;
+celix_status_t celix_utils_findIpInSubnet(const char* subnetCidrNotation,
char** foundIp) {
+ assert(subnetCidrNotation);
+ assert(foundIp);
+
+ *foundIp = NULL;
- char *input = celix_utils_strdup(ipWithPrefix); // Make a copy as
otherwise strtok_r manipulates the input string
- if (input == NULL) {
- goto strdup_failed;
+ //create copy for strtok_r
+ celix_autofree char* input = celix_utils_strdup(subnetCidrNotation);
+ if (!input) {
+ celix_err_push("Failed to duplicate input string for subnet search");
+ return CELIX_ENOMEM;
}
- char *savePtr;
- char *inputIp = strtok_r(input, "/", &savePtr);
- char *inputPrefixStr = strtok_r(NULL, "\0", &savePtr);
- unsigned int inputPrefix = (unsigned int) strtoul(inputPrefixStr, NULL,
10);
+ char* savePtr;
+ char* inputIp = strtok_r(input, "/", &savePtr);
+ char* inputPrefixStr = strtok_r(NULL, "\0", &savePtr);
- unsigned int ipAsUint = ipUtils_ipToUnsignedInt(inputIp);
- unsigned int bitmask = ipUtils_prefixToBitmask(inputPrefix);
+ if (!inputPrefixStr) {
+ celix_err_pushf("Failed to parse IP address with prefix %s. Missing a
'/'", subnetCidrNotation);
+ return CELIX_ILLEGAL_ARGUMENT;
+ }
+
+ bool convertedLong = false;
+ int inputPrefix = (int)celix_utils_convertStringToLong(inputPrefixStr,
INT_MAX, &convertedLong);
+ if (!convertedLong) {
+ celix_err_pushf("Failed to parse prefix in IP address with prefix %s",
subnetCidrNotation);
+ return CELIX_ILLEGAL_ARGUMENT;
+ } else if (inputPrefix > 32 || inputPrefix < 0) {
+ celix_err_pushf(
+ "Failed to parse IP address with prefix %s. Prefix %s is out of
range", subnetCidrNotation, inputPrefixStr);
+ return CELIX_ILLEGAL_ARGUMENT;
+ }
+
+ bool converted;
+ uint32_t ipAsUint = celix_utils_convertIpToUint(inputIp, &converted);
+ if (!converted) {
+ return CELIX_ILLEGAL_ARGUMENT;
+ }
+ uint32_t bitmask = celix_utils_ipPrefixLengthToBitmask(inputPrefix);
unsigned int ipRangeStart = ipAsUint & bitmask;
unsigned int ipRangeStop = ipAsUint | ~bitmask;
// Requested IP range is known now, now loop through network interfaces
struct ifaddrs *ifap, *ifa;
- if(getifaddrs (&ifap) == -1) {
- goto getifaddrs_failed;
+ if (getifaddrs(&ifap) == -1) {
+ celix_status_t status = errno;
+ celix_err_push("Failed to get network interfaces");
+ return status;
}
+
for (ifa = ifap; ifa; ifa = ifa->ifa_next) {
- if (ifa->ifa_addr == NULL)
+ if (ifa->ifa_addr == NULL) {
continue;
+ }
- if (ifa->ifa_addr->sa_family != AF_INET)
+ if (ifa->ifa_addr->sa_family != AF_INET) {
continue;
+ }
// Retrieve IP address for interface
char if_addr[NI_MAXHOST];
- int rv = getnameinfo(ifa->ifa_addr, sizeof(struct sockaddr_in),
- if_addr, NI_MAXHOST, NULL, 0, NI_NUMERICHOST);
+ int rv = getnameinfo(ifa->ifa_addr, sizeof(struct sockaddr_in),
if_addr, NI_MAXHOST, NULL, 0, NI_NUMERICHOST);
if (rv != 0) {
- printf("getnameinfo() failed: %s\n", gai_strerror(rv));
+ celix_err_pushf("Failed to get IP address for interface %s: %s",
ifa->ifa_name, gai_strerror(rv));
continue;
}
// Retrieve netmask
- struct sockaddr_in *sa = (struct sockaddr_in *) ifa->ifa_netmask;
- char *if_netmask = inet_ntoa(sa->sin_addr);
+ struct sockaddr_in* sa = (struct sockaddr_in*)ifa->ifa_netmask;
+ char* if_netmask = inet_ntoa(sa->sin_addr);
Review Comment:
`inet_ntoa` uses statically allocated buffer, and thus IMO should be
avoided.
Moreover, if `celix_utils_ipNetmaskToPrefixLength` accepts `uint32_t` rather
than `const char*`, then this is unnecessary.
##########
libs/utils/src/ip_utils.c:
##########
@@ -16,155 +16,207 @@
* specific language governing permissions and limitations
* under the License.
*/
-/**
- * ip_utils.c
- *
- * \date Jun 24, 2019
- * \author <a href="mailto:dev@celix.apache.org";>Apache Celix Project
Team</a>
- * \copyright Apache License, Version 2.0
- */
-#include "ip_utils.h"
+#include "celix_ip_utils.h"
+
+#include "celix_err.h"
+#include "celix_stdlib_cleanup.h"
#include "celix_utils.h"
+#include "celix_convert_utils.h"
+#include <arpa/inet.h>
+#include <assert.h>
#include <errno.h>
+#include <ifaddrs.h>
+#include <limits.h>
+#include <math.h>
+#include <netdb.h>
+#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>
-#include <stdbool.h>
#include <string.h>
-#include <math.h>
-
-#include <arpa/inet.h>
#include <sys/socket.h>
-#include <netdb.h>
-#include <ifaddrs.h>
-#include <unistd.h>
-unsigned int ipUtils_ipToUnsignedInt(char *ip) {
- unsigned int ipAsUint = 0;
+uint32_t celix_utils_convertIpToUint(const char* ip, bool* converted) {
+ if (converted) {
+ *converted = false;
+ }
- char *partOfIp = NULL, *savePtr = NULL;
- char *input = strdup(ip); // Make a copy because otherwise strtok_r
manipulates the input string
- partOfIp = strtok_r(input, ".\0", &savePtr); ipAsUint += strtoul(partOfIp,
NULL, 10) * (unsigned int) pow(256, 3);
- partOfIp = strtok_r(NULL, ".\0", &savePtr); ipAsUint += strtoul(partOfIp,
NULL, 10) * (unsigned int) pow(256, 2);
- partOfIp = strtok_r(NULL, ".\0", &savePtr); ipAsUint += strtoul(partOfIp,
NULL, 10) * (unsigned int) pow(256, 1);
- partOfIp = strtok_r(NULL, ".\0", &savePtr); ipAsUint += strtoul(partOfIp,
NULL, 10) * (unsigned int) pow(256, 0);
- free(input);
+ // copy for strtok_r
+ celix_autofree char* input = celix_utils_strdup(ip);
+ if (!input) {
+ celix_err_push("Failed to duplicate input string for IP conversion");
+ return 0;
+ }
+ uint32_t ipAsUint = 0;
+ char* partOfIp = NULL;
+ char* savePtr = NULL;
+ int count = 0;
+ while ((partOfIp = strtok_r(partOfIp == NULL ? input : NULL, ".\0",
&savePtr)) != NULL) {
+ if (count > 3) {
+ celix_err_pushf("Failed to convert IP address %s to unsigned int,
to many parts", ip);
+ return 0;
+ }
+ bool longConverted = false;
+ long partAsLong = celix_utils_convertStringToLong(partOfIp, ULONG_MAX,
&longConverted);
+ if (!longConverted) {
+ celix_err_pushf("Failed to convert IP address %s to unsigned int,
part `%s` is not a number", ip, partOfIp);
+ return 0;
+ } else if (partAsLong > 255 || partAsLong < 0) {
+ celix_err_pushf("Failed to convert IP address %s to unsigned int,
part `%s` is out of range", ip, partOfIp);
+ return 0;
+ }
+ ipAsUint += (uint32_t)(partAsLong * powl(256, 3 - count++));
+ }
+
+ if (converted) {
+ *converted = true;
+ }
return ipAsUint;
}
-char *ipUtils_unsignedIntToIp(unsigned int ip) {
- char *ipStr = calloc(16, sizeof(char));
+char* celix_utils_convertUintToIp(uint32_t ip) {
+ char* ipStr = calloc(16, sizeof(char));
+ if (!ipStr) {
+ celix_err_push("Failed to allocate memory for IP address string");
+ return NULL;
+ }
- int ipPart1 = ip / (int) pow(256, 3); ip -= ipPart1 * (int) pow(256, 3);
- int ipPart2 = ip / (int) pow(256, 2); ip -= ipPart2 * (int) pow(256, 2);
- int ipPart3 = ip / (int) pow(256, 1); ip -= ipPart3 * (int) pow(256, 1);
- int ipPart4 = ip / (int) pow(256, 0);
+ int64_t ipPart1 = ip / (int64_t)pow(256, 3);
+ ip -= ipPart1 * (int64_t)pow(256, 3);
+ int64_t ipPart2 = ip / (int64_t)pow(256, 2);
+ ip -= ipPart2 * (int64_t)pow(256, 2);
+ int64_t ipPart3 = ip / (int64_t)pow(256, 1);
+ ip -= ipPart3 * (int64_t)pow(256, 1);
+ int64_t ipPart4 = ip / (int64_t)pow(256, 0);
- snprintf(ipStr, 16, "%d.%d.%d.%d", ipPart1, ipPart2, ipPart3, ipPart4);
+ snprintf(ipStr, 16, "%i.%i.%i.%i", (int)ipPart1, (int)ipPart2,
(int)ipPart3, (int)ipPart4);
return ipStr;
}
-unsigned int ipUtils_prefixToBitmask(unsigned int prefix) {
- return (0xFFFFFFFF << (32 - prefix)) & 0xFFFFFFFF;
+uint32_t celix_utils_ipPrefixLengthToBitmask(int prefix) {
+ if (prefix > 32 || prefix <= 0) {
+ return 0;
+ }
+ return (uint32_t )((0xFFFFFFFF << (32 - prefix)) & 0xFFFFFFFF);
}
-int ipUtils_netmaskToPrefix(const char *netmask) {
+int celix_utils_ipNetmaskToPrefixLength(const char* netmask) {
// Convert netmask to in_addr object
struct in_addr in;
int ret = inet_pton(AF_INET, netmask, &in);
if (ret != 1) {
+ celix_err_pushf("Failed to convert netmask %s to in_addr object",
netmask);
return -1;
}
// Now convert the mask to a prefix
int prefix = 0;
- bool processed_one = false;
- unsigned int i = ntohl(in.s_addr);
+ uint32_t i = ntohl(in.s_addr);
while (i > 0) {
if (i & 1) {
prefix++;
- processed_one = true;
- } else {
- if (processed_one) return -1;
}
-
i >>= 1;
}
return prefix;
}
-/** Finds an IP of the available network interfaces of the machine by
specifying an CIDR subnet.
- *
- * @param ipWithPrefix IP with prefix, e.g. 192.168.1.0/24
- * @return ip In case a matching interface could be found, an
allocated string containing the IP of the
- * interface will be returned, e.g. 192.168.1.16. Memory
for the new string can be freed with free().
- * When no matching interface is found NULL will be
returned.
- */
-char *ipUtils_findIpBySubnet(const char *ipWithPrefix) {
- char *ip = NULL;
+celix_status_t celix_utils_findIpInSubnet(const char* subnetCidrNotation,
char** foundIp) {
+ assert(subnetCidrNotation);
+ assert(foundIp);
+
+ *foundIp = NULL;
- char *input = celix_utils_strdup(ipWithPrefix); // Make a copy as
otherwise strtok_r manipulates the input string
- if (input == NULL) {
- goto strdup_failed;
+ //create copy for strtok_r
+ celix_autofree char* input = celix_utils_strdup(subnetCidrNotation);
+ if (!input) {
+ celix_err_push("Failed to duplicate input string for subnet search");
+ return CELIX_ENOMEM;
}
- char *savePtr;
- char *inputIp = strtok_r(input, "/", &savePtr);
- char *inputPrefixStr = strtok_r(NULL, "\0", &savePtr);
- unsigned int inputPrefix = (unsigned int) strtoul(inputPrefixStr, NULL,
10);
+ char* savePtr;
+ char* inputIp = strtok_r(input, "/", &savePtr);
+ char* inputPrefixStr = strtok_r(NULL, "\0", &savePtr);
- unsigned int ipAsUint = ipUtils_ipToUnsignedInt(inputIp);
- unsigned int bitmask = ipUtils_prefixToBitmask(inputPrefix);
+ if (!inputPrefixStr) {
+ celix_err_pushf("Failed to parse IP address with prefix %s. Missing a
'/'", subnetCidrNotation);
Review Comment:
Missing a '/' may not be accurate. For exmaple,
```C++
EXPECT_EQ(CELIX_ILLEGAL_ARGUMENT, celix_utils_findIpInSubnet("/",
&ipAddr)); // empty address
EXPECT_EQ(1, celix_err_getErrorCount());
```
##########
libs/utils/src/ip_utils.c:
##########
@@ -16,155 +16,207 @@
* specific language governing permissions and limitations
* under the License.
*/
-/**
- * ip_utils.c
- *
- * \date Jun 24, 2019
- * \author <a href="mailto:dev@celix.apache.org";>Apache Celix Project
Team</a>
- * \copyright Apache License, Version 2.0
- */
-#include "ip_utils.h"
+#include "celix_ip_utils.h"
+
+#include "celix_err.h"
+#include "celix_stdlib_cleanup.h"
#include "celix_utils.h"
+#include "celix_convert_utils.h"
+#include <arpa/inet.h>
+#include <assert.h>
#include <errno.h>
+#include <ifaddrs.h>
+#include <limits.h>
+#include <math.h>
+#include <netdb.h>
+#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>
-#include <stdbool.h>
#include <string.h>
-#include <math.h>
-
-#include <arpa/inet.h>
#include <sys/socket.h>
-#include <netdb.h>
-#include <ifaddrs.h>
-#include <unistd.h>
-unsigned int ipUtils_ipToUnsignedInt(char *ip) {
- unsigned int ipAsUint = 0;
+uint32_t celix_utils_convertIpToUint(const char* ip, bool* converted) {
+ if (converted) {
+ *converted = false;
+ }
- char *partOfIp = NULL, *savePtr = NULL;
- char *input = strdup(ip); // Make a copy because otherwise strtok_r
manipulates the input string
- partOfIp = strtok_r(input, ".\0", &savePtr); ipAsUint += strtoul(partOfIp,
NULL, 10) * (unsigned int) pow(256, 3);
- partOfIp = strtok_r(NULL, ".\0", &savePtr); ipAsUint += strtoul(partOfIp,
NULL, 10) * (unsigned int) pow(256, 2);
- partOfIp = strtok_r(NULL, ".\0", &savePtr); ipAsUint += strtoul(partOfIp,
NULL, 10) * (unsigned int) pow(256, 1);
- partOfIp = strtok_r(NULL, ".\0", &savePtr); ipAsUint += strtoul(partOfIp,
NULL, 10) * (unsigned int) pow(256, 0);
- free(input);
+ // copy for strtok_r
+ celix_autofree char* input = celix_utils_strdup(ip);
+ if (!input) {
+ celix_err_push("Failed to duplicate input string for IP conversion");
+ return 0;
+ }
+ uint32_t ipAsUint = 0;
+ char* partOfIp = NULL;
+ char* savePtr = NULL;
+ int count = 0;
+ while ((partOfIp = strtok_r(partOfIp == NULL ? input : NULL, ".\0",
&savePtr)) != NULL) {
+ if (count > 3) {
+ celix_err_pushf("Failed to convert IP address %s to unsigned int,
to many parts", ip);
+ return 0;
+ }
+ bool longConverted = false;
+ long partAsLong = celix_utils_convertStringToLong(partOfIp, ULONG_MAX,
&longConverted);
+ if (!longConverted) {
+ celix_err_pushf("Failed to convert IP address %s to unsigned int,
part `%s` is not a number", ip, partOfIp);
+ return 0;
+ } else if (partAsLong > 255 || partAsLong < 0) {
+ celix_err_pushf("Failed to convert IP address %s to unsigned int,
part `%s` is out of range", ip, partOfIp);
+ return 0;
+ }
+ ipAsUint += (uint32_t)(partAsLong * powl(256, 3 - count++));
+ }
+
+ if (converted) {
+ *converted = true;
+ }
return ipAsUint;
}
-char *ipUtils_unsignedIntToIp(unsigned int ip) {
- char *ipStr = calloc(16, sizeof(char));
+char* celix_utils_convertUintToIp(uint32_t ip) {
+ char* ipStr = calloc(16, sizeof(char));
+ if (!ipStr) {
+ celix_err_push("Failed to allocate memory for IP address string");
+ return NULL;
+ }
- int ipPart1 = ip / (int) pow(256, 3); ip -= ipPart1 * (int) pow(256, 3);
- int ipPart2 = ip / (int) pow(256, 2); ip -= ipPart2 * (int) pow(256, 2);
- int ipPart3 = ip / (int) pow(256, 1); ip -= ipPart3 * (int) pow(256, 1);
- int ipPart4 = ip / (int) pow(256, 0);
+ int64_t ipPart1 = ip / (int64_t)pow(256, 3);
+ ip -= ipPart1 * (int64_t)pow(256, 3);
+ int64_t ipPart2 = ip / (int64_t)pow(256, 2);
+ ip -= ipPart2 * (int64_t)pow(256, 2);
+ int64_t ipPart3 = ip / (int64_t)pow(256, 1);
+ ip -= ipPart3 * (int64_t)pow(256, 1);
+ int64_t ipPart4 = ip / (int64_t)pow(256, 0);
- snprintf(ipStr, 16, "%d.%d.%d.%d", ipPart1, ipPart2, ipPart3, ipPart4);
+ snprintf(ipStr, 16, "%i.%i.%i.%i", (int)ipPart1, (int)ipPart2,
(int)ipPart3, (int)ipPart4);
return ipStr;
}
-unsigned int ipUtils_prefixToBitmask(unsigned int prefix) {
- return (0xFFFFFFFF << (32 - prefix)) & 0xFFFFFFFF;
+uint32_t celix_utils_ipPrefixLengthToBitmask(int prefix) {
+ if (prefix > 32 || prefix <= 0) {
+ return 0;
+ }
+ return (uint32_t )((0xFFFFFFFF << (32 - prefix)) & 0xFFFFFFFF);
}
-int ipUtils_netmaskToPrefix(const char *netmask) {
+int celix_utils_ipNetmaskToPrefixLength(const char* netmask) {
// Convert netmask to in_addr object
struct in_addr in;
int ret = inet_pton(AF_INET, netmask, &in);
if (ret != 1) {
+ celix_err_pushf("Failed to convert netmask %s to in_addr object",
netmask);
return -1;
}
// Now convert the mask to a prefix
int prefix = 0;
- bool processed_one = false;
- unsigned int i = ntohl(in.s_addr);
+ uint32_t i = ntohl(in.s_addr);
while (i > 0) {
if (i & 1) {
prefix++;
- processed_one = true;
- } else {
- if (processed_one) return -1;
}
-
i >>= 1;
}
return prefix;
}
-/** Finds an IP of the available network interfaces of the machine by
specifying an CIDR subnet.
- *
- * @param ipWithPrefix IP with prefix, e.g. 192.168.1.0/24
- * @return ip In case a matching interface could be found, an
allocated string containing the IP of the
- * interface will be returned, e.g. 192.168.1.16. Memory
for the new string can be freed with free().
- * When no matching interface is found NULL will be
returned.
- */
-char *ipUtils_findIpBySubnet(const char *ipWithPrefix) {
- char *ip = NULL;
+celix_status_t celix_utils_findIpInSubnet(const char* subnetCidrNotation,
char** foundIp) {
+ assert(subnetCidrNotation);
+ assert(foundIp);
+
+ *foundIp = NULL;
- char *input = celix_utils_strdup(ipWithPrefix); // Make a copy as
otherwise strtok_r manipulates the input string
- if (input == NULL) {
- goto strdup_failed;
+ //create copy for strtok_r
+ celix_autofree char* input = celix_utils_strdup(subnetCidrNotation);
+ if (!input) {
+ celix_err_push("Failed to duplicate input string for subnet search");
+ return CELIX_ENOMEM;
}
- char *savePtr;
- char *inputIp = strtok_r(input, "/", &savePtr);
- char *inputPrefixStr = strtok_r(NULL, "\0", &savePtr);
- unsigned int inputPrefix = (unsigned int) strtoul(inputPrefixStr, NULL,
10);
+ char* savePtr;
+ char* inputIp = strtok_r(input, "/", &savePtr);
+ char* inputPrefixStr = strtok_r(NULL, "\0", &savePtr);
- unsigned int ipAsUint = ipUtils_ipToUnsignedInt(inputIp);
- unsigned int bitmask = ipUtils_prefixToBitmask(inputPrefix);
+ if (!inputPrefixStr) {
+ celix_err_pushf("Failed to parse IP address with prefix %s. Missing a
'/'", subnetCidrNotation);
+ return CELIX_ILLEGAL_ARGUMENT;
+ }
+
+ bool convertedLong = false;
+ int inputPrefix = (int)celix_utils_convertStringToLong(inputPrefixStr,
INT_MAX, &convertedLong);
+ if (!convertedLong) {
+ celix_err_pushf("Failed to parse prefix in IP address with prefix %s",
subnetCidrNotation);
+ return CELIX_ILLEGAL_ARGUMENT;
+ } else if (inputPrefix > 32 || inputPrefix < 0) {
+ celix_err_pushf(
+ "Failed to parse IP address with prefix %s. Prefix %s is out of
range", subnetCidrNotation, inputPrefixStr);
+ return CELIX_ILLEGAL_ARGUMENT;
+ }
+
+ bool converted;
+ uint32_t ipAsUint = celix_utils_convertIpToUint(inputIp, &converted);
+ if (!converted) {
+ return CELIX_ILLEGAL_ARGUMENT;
+ }
+ uint32_t bitmask = celix_utils_ipPrefixLengthToBitmask(inputPrefix);
unsigned int ipRangeStart = ipAsUint & bitmask;
unsigned int ipRangeStop = ipAsUint | ~bitmask;
// Requested IP range is known now, now loop through network interfaces
struct ifaddrs *ifap, *ifa;
- if(getifaddrs (&ifap) == -1) {
- goto getifaddrs_failed;
+ if (getifaddrs(&ifap) == -1) {
+ celix_status_t status = errno;
+ celix_err_push("Failed to get network interfaces");
+ return status;
}
+
for (ifa = ifap; ifa; ifa = ifa->ifa_next) {
- if (ifa->ifa_addr == NULL)
+ if (ifa->ifa_addr == NULL) {
continue;
+ }
- if (ifa->ifa_addr->sa_family != AF_INET)
+ if (ifa->ifa_addr->sa_family != AF_INET) {
continue;
+ }
// Retrieve IP address for interface
char if_addr[NI_MAXHOST];
- int rv = getnameinfo(ifa->ifa_addr, sizeof(struct sockaddr_in),
- if_addr, NI_MAXHOST, NULL, 0, NI_NUMERICHOST);
+ int rv = getnameinfo(ifa->ifa_addr, sizeof(struct sockaddr_in),
if_addr, NI_MAXHOST, NULL, 0, NI_NUMERICHOST);
Review Comment:
We already have `ifa->ifa_addr` ipv4 address, why bother calling
`getnameinfo`?
The following `celix_utils_convertIpToUint` seems unnecessary.
##########
libs/utils/src/hash_map_private.h:
##########
@@ -29,13 +29,17 @@
#include "hash_map.h"
-unsigned int hashMap_hashCode(const void* toHash);
-int hashMap_equals(const void* toCompare, const void* compare);
+#ifdef __cplusplus
+extern "C" {
+#endif
-void hashMap_resize(hash_map_pt map, int newCapacity);
-hash_map_entry_pt hashMap_removeEntryForKey(hash_map_pt map, const void* key);
-hash_map_entry_pt hashMap_removeMapping(hash_map_pt map, hash_map_entry_pt
entry);
-void hashMap_addEntry(hash_map_pt map, int hash, void* key, void* value, int
bucketIndex);
+CELIX_UTILS_DEPRECATED_EXPORT unsigned int hashMap_hashCode(const void*
toHash);
Review Comment:
Why export? `CELIX_UTILS_DEPRECATED` seems more reasonable.
--
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
