pnoltes commented on code in PR #495: URL: https://github.com/apache/celix/pull/495#discussion_r1143554339
########## bundles/remote_services/discovery_zeroconf/src/discovery_zeroconf_watcher.c: ########## @@ -0,0 +1,587 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +#include <discovery_zeroconf_watcher.h> +#include <discovery_zeroconf_constants.h> +#include <dns_sd.h> +#include <endpoint_listener.h> +#include <remote_constants.h> +#include <celix_bundle_context.h> +#include <celix_filter.h> +#include <celix_constants.h> +#include <celix_long_hash_map.h> +#include <celix_string_hash_map.h> +#include <celix_threads.h> +#include <celix_utils.h> +#include <celix_errno.h> +#include <sys/eventfd.h> +#include <sys/select.h> +#include <sys/param.h> +#include <stdlib.h> +#include <unistd.h> +#include <stdbool.h> +#include <string.h> +#include <limits.h> +#include <assert.h> +#include <errno.h> + +#define DZC_EP_JITTER_INTERVAL 10 +#define DZC_MAX_RESOLVED_TIMEOUT 5 +#define DZC_MAX_RESOLVED_CNT 10 + +struct discovery_zeroconf_watcher { + celix_bundle_context_t *ctx; + celix_log_helper_t *logHelper; + long epListenerTrkId; + char fwUuid[64]; + DNSServiceRef sharedRef; + DNSServiceRef browseRef; + int eventFd; + celix_thread_t watchEPThread; + celix_string_hash_map_t *watchedServices;//key:instanceName+interfaceId, val:watched_service_entry_t* + celix_thread_mutex_t mutex;//projects below + bool running; + celix_string_hash_map_t *watchedEndpoints;//key:endpoint id, val:watched_endpoint_entry_t* + celix_long_hash_map_t *epls;//key:service id, val:endpoint listener +}; + +typedef struct watched_endpoint_entry { + endpoint_description_t *endpoint; + struct timespec expiredTime; +}watched_endpoint_entry_t; + +typedef struct watched_service_entry { + celix_properties_t *txtRecord; + const char *endpointId; + int ifIndex; + char instanceName[64];//The instanceName must be 1-63 bytes + bool resolved; + struct timespec resolvedStartTime; + int resolvedCnt; + DNSServiceRef resolveRef; +}watched_service_entry_t; + +typedef struct watched_epl_entry { + endpoint_listener_t *epl; + celix_filter_t *filter; +}watched_epl_entry_t; + +static void discoveryZeroconfWatcher_addEPL(void *handle, void *svc, const celix_properties_t *props); +static void discoveryZeroconfWatcher_removeEPL(void *handle, void *svc, const celix_properties_t *props); +static void OnServiceResolveCallback(DNSServiceRef sdRef, DNSServiceFlags flags, uint32_t interfaceIndex, DNSServiceErrorType errorCode, const char *fullname, const char *host, uint16_t port, uint16_t txtLen, const unsigned char *txtRecord, void *context); +static void OnServiceBrowseCallback(DNSServiceRef sdRef, DNSServiceFlags flags, uint32_t interfaceIndex, DNSServiceErrorType errorCode, const char *instanceName, const char *regtype, const char *replyDomain, void *context); +static void *discoveryZeroconfWatcher_watchEPThread(void *data); + +celix_status_t discoveryZeroconfWatcher_create(celix_bundle_context_t *ctx, celix_log_helper_t *logHelper, discovery_zeroconf_watcher_t **watcherOut) { + celix_status_t status = CELIX_SUCCESS; + discovery_zeroconf_watcher_t *watcher = (discovery_zeroconf_watcher_t *)calloc(1, sizeof(*watcher)); + if (watcher == NULL) { + celix_logHelper_fatal(logHelper, "Watcher: Failed to alloc watcher."); + status = CELIX_ENOMEM; + goto watcher_err; + } + watcher->logHelper = logHelper; + watcher->ctx = ctx; + watcher->sharedRef = NULL; + watcher->browseRef = NULL; + watcher->eventFd = eventfd(0, 0); + if (watcher->eventFd < 0) { + status = CELIX_ERROR_MAKE(CELIX_FACILITY_CERRNO, errno); + celix_logHelper_fatal(logHelper, "Watcher: Failed to open event fd, %d.", errno); + goto event_fd_err; + } + + const char *fwUuid = celix_bundleContext_getProperty(ctx, OSGI_FRAMEWORK_FRAMEWORK_UUID, NULL); + if (fwUuid == NULL || strlen(fwUuid) >= sizeof(watcher->fwUuid)) { + status = CELIX_BUNDLE_EXCEPTION; + celix_logHelper_fatal(logHelper, "Watcher: Failed to get framework uuid."); + goto fw_uuid_err; + } + strcpy(watcher->fwUuid, fwUuid); + + status = celixThreadMutex_create(&watcher->mutex, NULL); + if (status != CELIX_SUCCESS) { + celix_logHelper_fatal(logHelper, "Watcher: Failed to create mutex, %d.", status); + goto mutex_err; + } + celix_string_hash_map_create_options_t epOpts = CELIX_EMPTY_STRING_HASH_MAP_CREATE_OPTIONS; + epOpts.storeKeysWeakly = true; + watcher->watchedEndpoints = celix_stringHashMap_createWithOptions(&epOpts); + assert(watcher->watchedEndpoints); + celix_string_hash_map_create_options_t svcOpts = CELIX_EMPTY_STRING_HASH_MAP_CREATE_OPTIONS; + watcher->watchedServices = celix_stringHashMap_createWithOptions(&svcOpts); + assert(watcher->watchedServices != NULL); + + watcher->epls = celix_longHashMap_create(); + assert(watcher->epls != NULL); + + celix_service_tracking_options_t opts = CELIX_EMPTY_SERVICE_TRACKING_OPTIONS; + opts.filter.serviceName = OSGI_ENDPOINT_LISTENER_SERVICE; + opts.filter.filter = "(!(DISCOVERY=true))"; Review Comment: I think the correct way of doing this is to not process discovered endpoint with matching framwork uuid ########## bundles/remote_services/discovery_zeroconf/src/discovery_zeroconf_announcer.c: ########## @@ -0,0 +1,547 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +#include <discovery_zeroconf_announcer.h> +#include <discovery_zeroconf_constants.h> +#include <dns_sd.h> +#include <endpoint_listener.h> +#include <remote_constants.h> +#include <celix_utils.h> +#include <celix_properties.h> +#include <celix_constants.h> +#include <celix_threads.h> +#include <celix_bundle_context.h> +#include <celix_string_hash_map.h> +#include <celix_array_list.h> +#include <celix_log_helper.h> +#include <celix_types.h> +#include <celix_errno.h> +#include <celix_build_assert.h> +#include <netinet/in.h> +#include <net/if.h> +#include <sys/eventfd.h> +#include <sys/select.h> +#include <sys/param.h> +#include <sys/socket.h> +#include <sys/ioctl.h> +#include <unistd.h> +#include <stdlib.h> +#include <assert.h> +#include <stdio.h> +#include <stdbool.h> +#include <stdint.h> +#include <stddef.h> +#include <string.h> +#include <errno.h> + + +#define DZC_MAX_CONFLICT_CNT 256 + +//According to rfc6763, Using TXT records larger than 1300 bytes is NOT RECOMMENDED +#define DZC_MAX_TXT_RECORD_SIZE 1300 + +struct discovery_zeroconf_announcer { + celix_bundle_context_t *ctx; + celix_log_helper_t *logHelper; + char fwUuid[64]; + endpoint_listener_t epListener; + long epListenerSvcId; + DNSServiceRef sharedRef; + int eventFd; + celix_thread_t refreshEPThread; + celix_thread_mutex_t mutex;//projects below + bool running; + celix_string_hash_map_t *endpoints;//key:endpoint id, val:announce_endpoint_entry_t* + celix_array_list_t *revokedEndpoints; +}; + +typedef struct announce_endpoint_entry { + celix_properties_t *properties; + DNSServiceRef registerRef; + unsigned int uid; + int ifIndex; + const char *serviceName; + char serviceType[64]; + bool announced; +}announce_endpoint_entry_t; + + +static void discoveryZeroconfAnnouncer_eventNotify(discovery_zeroconf_announcer_t *announcer); +static celix_status_t discoveryZeroconfAnnouncer_endpointAdded(void *handle, endpoint_description_t *endpoint, char *matchedFilter); +static celix_status_t discoveryZeroconfAnnouncer_endpointRemoved(void *handle, endpoint_description_t *endpoint, char *matchedFilter); +static void *discoveryZeroconfAnnouncer_refreshEndpointThread(void *data); + + +celix_status_t discoveryZeroconfAnnouncer_create(celix_bundle_context_t *ctx, celix_log_helper_t *logHelper, discovery_zeroconf_announcer_t **announcerOut) { + celix_status_t status = CELIX_SUCCESS; + discovery_zeroconf_announcer_t *announcer = (discovery_zeroconf_announcer_t *)calloc(1, sizeof(*announcer)); + if (announcer == NULL) { + celix_logHelper_fatal(logHelper, "Announcer: Failed to alloc announcer."); + status = CELIX_ENOMEM; + goto announcer_err; + } + announcer->ctx = ctx; + announcer->logHelper = logHelper; + announcer->sharedRef = NULL; + + announcer->eventFd = eventfd(0, 0); + if (announcer->eventFd < 0) { + status = CELIX_ERROR_MAKE(CELIX_FACILITY_CERRNO, errno); + celix_logHelper_fatal(logHelper, "Announcer: Failed to open event fd, %d.", errno); + goto eventfd_err; + } + + status = celixThreadMutex_create(&announcer->mutex, NULL); + if (status != CELIX_SUCCESS) { + celix_logHelper_fatal(logHelper, "Announcer: Failed to create mutex, %d.", status); + goto mutex_err; + } + + announcer->endpoints = celix_stringHashMap_create(); + assert(announcer->endpoints != NULL); + announcer->revokedEndpoints = celix_arrayList_create(); + assert(announcer->revokedEndpoints != NULL); + + const char *fwUuid = celix_bundleContext_getProperty(ctx, OSGI_FRAMEWORK_FRAMEWORK_UUID, NULL); + if (fwUuid == NULL || strlen(fwUuid) >= sizeof(announcer->fwUuid)) { + status = CELIX_BUNDLE_EXCEPTION; + celix_logHelper_fatal(logHelper, "Announcer: Failed to get framework uuid."); + goto fw_uuid_err; + } + strcpy(announcer->fwUuid, fwUuid); + + announcer->epListener.handle = announcer; + announcer->epListener.endpointAdded = discoveryZeroconfAnnouncer_endpointAdded; + announcer->epListener.endpointRemoved = discoveryZeroconfAnnouncer_endpointRemoved; + char scope[256] = {0}; + (void)snprintf(scope, sizeof(scope), "(&(%s=*)(%s=%s))", OSGI_FRAMEWORK_OBJECTCLASS, OSGI_RSA_ENDPOINT_FRAMEWORK_UUID, fwUuid); + + celix_properties_t *props = celix_properties_create(); + assert(props != NULL); + celix_properties_set(props, "DISCOVERY", "true"); Review Comment: I see that the "DISCOVERY" property is used to distinguish from endpoint listener for announcement and for discovery. If I remember correctly, the "correct" way to do this is to only process endpoints which matches the framework uuid and vica versa for the watcher (only process endpoint which do not match the framework uuid). To be honest, I would have preferred 2 separate interfaces (endpoint announcement listener and discovered endpoint listener) as is done with pubsub, but this is not part of the specification (https://docs.osgi.org/specification/osgi.cmpn/7.0.0/service.remoteserviceadmin.html) ########## bundles/remote_services/rsa_spi/include/remote_constants.h: ########## @@ -36,4 +36,6 @@ static const char * const OSGI_RSA_SERVICE_IMPORTED_CONFIGS = "service.imported. static const char * const OSGI_RSA_SERVICE_EXPORTED_CONFIGS = "service.exported.configs"; static const char * const OSGI_RSA_SERVICE_LOCATION = "service.location"; +static const char * const RSA_DISCOVERY_ZEROCONF_SERVICE_ANNOUNCED_IF_INDEX = "RSA_DZC_IF_INDEX"; Review Comment: Additional note: The requested change is quite big, so for me it is also fine to accept the current solution and maybe in the future improve this. ########## bundles/remote_services/rsa_spi/include/remote_constants.h: ########## @@ -36,4 +36,6 @@ static const char * const OSGI_RSA_SERVICE_IMPORTED_CONFIGS = "service.imported. static const char * const OSGI_RSA_SERVICE_EXPORTED_CONFIGS = "service.exported.configs"; static const char * const OSGI_RSA_SERVICE_LOCATION = "service.location"; +static const char * const RSA_DISCOVERY_ZEROCONF_SERVICE_ANNOUNCED_IF_INDEX = "RSA_DZC_IF_INDEX"; Review Comment: Based on the OSGi specification, this can be used on providing service properties: - service.exported.configs - service.exported.intents - service.exported.intents.extra - service.exported.interfaces - service.intents - service.pid See https://docs.osgi.org/specification/osgi.cmpn/8.0.0/service.remoteservices.html. So this is very technology agnostic (as it should be). So I think this should be a configuration for the Remote Service Admin and the maybe the discovery part. What about using something like CELIX_REMOTE_SERVICES_SUBNETS - as mentiond above - which configures the remote service admin and discovery which network interface to use. For the configured network interfaces, the remote service admin will export and import services. To align this with discovery, I think the remote service admin needs to communicate (through endpoint properties) the target interface (for endpoint announcement) and the discovery announcement should remove this property (not make it public). It is the responsibility of the remote service admin to add endpoint info where it is reachable (i.e. ip of network interface). For endpoint discovery (watch) I think nothing extra is needed, because the counterpart remote service admin should provide info that is reachable. Note that with remote services (and this is different than the Apache Celix PubSub) you can have multiple service imports for different configuration, what have you. @xuzhenbao Does that help? ########## bundles/remote_services/doc/discovery_zeroconf.adoc: ########## Review Comment: nice to also have some documentation :+1: -- 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
