Copilot commented on code in PR #2176: URL: https://github.com/apache/nifi-minifi-cpp/pull/2176#discussion_r3247745572
########## extensions/stable-api-sandbox/ExtensionInitializer.cpp: ########## @@ -0,0 +1,39 @@ +/** + * 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 "../../extension-framework/cpp-extension-lib/include/api/core/Resource.h" Review Comment: This duplicate include of `Resource.h` via a relative path that escapes the source directory looks unintentional. The file already includes `api/core/Resource.h` on line 21 through the proper include path. The `../../extension-framework/...` path is fragile (depends on the file's exact location in the source tree, won't work from an out-of-tree extension that just links against the headers) and bypasses the configured include directories. Please remove the line 18 include. ########## minifi-api/include/minifi-c/minifi-c.h: ########## @@ -202,11 +203,18 @@ typedef struct MinifiProcessorClassDefinition { MinifiProcessorCallbacks callbacks; } MinifiProcessorClassDefinition; +struct MinifiControllerServiceProvidedAPI { + MinifiStringView type; + void*(*cast_api)(void*); +}; + typedef struct MinifiControllerServiceClassDefinition { MinifiStringView full_name; // '::'-delimited fully qualified name e.g. 'org::apache::nifi::minifi::extensions::gcp::GCPCredentialsControllerService MinifiStringView description; size_t class_properties_count; const MinifiPropertyDefinition* class_properties_ptr; + size_t provided_interfaces_count; + const MinifiStringView* provided_interfaces_ptr; Review Comment: This change adds two new fields to `MinifiControllerServiceClassDefinition` (`provided_interfaces_count`, `provided_interfaces_ptr`) and a new function pointer `get_interface` to `MinifiControllerServiceCallbacks`, both of which alter the size and layout of structs that C extensions populate. However, `MINIFI_API_VERSION` (minifi-c.h:49) has not been bumped. Per the existing convention, C extensions must export `MinifiApiVersion == MINIFI_API_VERSION`, and the agent rejects mismatches; without bumping the version, an old extension compiled against API version 3 will pass the version check but provide a struct of the old size, leaving `provided_interfaces_*` and `get_interface` as garbage that the agent will then dereference (e.g. the unconditional call at minifi-c.cpp:611). Please bump `MINIFI_API_VERSION`. ########## libminifi/src/minifi-c.cpp: ########## @@ -600,6 +608,12 @@ MinifiStatus MinifiProcessContextGetControllerService( *controller_service_out = static_cast<MinifiControllerService*>(c_controller_service->getImpl()); return MINIFI_STATUS_SUCCESS; } + auto interface_res = class_description.callbacks.get_interface(c_controller_service->getImpl(), controller_service_type); + if (!interface_res) { + return MINIFI_STATUS_VALIDATION_FAILED; + } + *controller_service_out = static_cast<MinifiControllerService*>(interface_res); + return MINIFI_STATUS_SUCCESS; Review Comment: `get_interface` is invoked unconditionally without first checking that the function pointer is non-null. A C extension that does not declare any provided interfaces has no reason to populate this callback, and `MinifiControllerServiceCallbacks` is documented (by its other use) as being aggregate-initialized field-by-field, so callers will commonly leave new optional fields at their default-initialized value. If `get_interface` is null this will crash. Please null-check the pointer (and return `MINIFI_STATUS_VALIDATION_FAILED` when absent) before calling it. ########## extensions/stable-api-sandbox/AnimalControllerServiceApis.h: ########## @@ -0,0 +1,33 @@ +/** + * 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. + */ + +#pragma once + +#include <cinttypes> + +namespace org::apache::nifi::minifi::api_sandbox { +class NumberOfLegsControllerApi { + public: + virtual uint8_t numberOfLegs() const = 0; +}; + +class CanFlyControllerApi { + public: Review Comment: `NumberOfLegsControllerApi` and `CanFlyControllerApi` are abstract interfaces meant to be inherited by concrete controller services and accessed polymorphically (the `cast` in `createProvidedInterface` returns an `Interface*`, and callers in `ZooProcessor.cpp` dereference it via that pointer). Without a virtual destructor in the interface, deleting through the base pointer would be undefined behavior. Even though current usage does not delete via the interface, please declare a `virtual ~NumberOfLegsControllerApi() = default;` (and likewise for `CanFlyControllerApi`) to make the contract safe and to match the polymorphic intent. ########## minifi-api/common/include/minifi-cpp/core/ProvidedControllerServiceInterface.h: ########## @@ -0,0 +1,33 @@ +/** + * 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. + */ +#pragma once + +#include <string_view> + +namespace org::apache::nifi::minifi::core { +struct ProvidedInterface { + std::string_view name; + void* (*cast)(void* instance); +}; + +template<typename Interface, typename ConcreteService> +constexpr ProvidedInterface createProvidedInterface() { + return ProvidedInterface{className<Interface>(), [](void* instance) -> void* { Review Comment: This header uses `core::className<Interface>()` but only includes `<string_view>`. It currently compiles only because translation units that include this header (e.g. `AnimalControllerServices.h`) happen to also pull in `core/ClassName.h` transitively via other headers. To make this header self-contained and robust, please add `#include "core/ClassName.h"`. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
