Dependency Refactoring: element retriction in api.git - #409
Conversation
Element restriction of Inference.Pipeline APIs should be done independently in api.git, not in nnstreamer.git This addresses nnstreamer/nnstreamer#3553 The next commit should address 1. unit testing: unittest_capi_inference.cc nnstreamer_capi_util element_available_* 2. packaging (install the .txt file along with api rpms) 3. removing restriction mechanisms from nnstreamer.git Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
|
📝 TAOS-CI Version: 1.5.20200925. Thank you for submitting PR #409. Please a submit 1commit/1PR (one commit per one PR) policy to get comments quickly from reviewers. Your PR must pass all verificiation processes of cibot before starting a review process from reviewers. If you are new member to join this project, please read manuals in documentation folder and wiki page. In order to monitor a progress status of your PR in more detail, visit http://ci.nnstreamer.ai/. |
|
|
|
|
myungjoo-bot
left a comment
There was a problem hiding this comment.
Automated review of this draft (transcribed from an AI review agent's report; please verify before acting).
Summary: The commit replaces the nnsconf_get_custom_value_*("element-restriction", ...) lookup in _ml_check_plugin_availability() with a hard-coded allow-list file /etc/ml_inference_pipeline_allowed_elements.txt parsed by new static helpers, so api.git no longer depends on nnstreamer's conf module (nnstreamer/nnstreamer#3553). The direction is still valid and not superseded: main's _ml_check_plugin_availability (c/src/ml-api-inference-pipeline.c:784-833) is identical to the merge-base version, git merge-tree is conflict-free, api.git still has no file-based mechanism, nnstreamer.git still carries the whole mechanism (enable-element-restriction, spec-generated nnstreamer.ini), and issue 3553 is still open. However, this 2023 sketch does not compile, and if it did the allow-check would reject every non-tensor_* element. It needs a rewrite with tests and packaging in the same PR or a stacked series.
All file:line references are PR-side c/src/ml-api-inference-pipeline.c.
- [High] Does not compile —
:844if (valid_lines_check => valid_lines):=>is not a C operator. Fix:>=. - [High] Stray semicolons make the allow-check always fail —
:878if (is_element_restriction_valid == FALSE);and:881if (is_element_restricted == FALSE);are empty statements, soreturn FALSE;at:879runs unconditionally. Every element exceptnnstreamer/tensor_*is reported restricted,ml_pipeline_constructreturnsML_ERROR_NOT_SUPPORTEDfor essentially every test pipeline, andml_check_element_availabilityreports everything unavailable. Fix: drop the semicolons; add a test constructingvideotestsrc ! tensor_converter ! fakesinkwith no allow-list present. - [High] printf argument mismatch —
:822: two%sbut onlyerr->messageis passed (filepathmissing); reads a garbage pointer at runtime. Fix: passfilepathtoo. - [High] Uninitialized
err/contentson the read-failure path —:800,:818-825:GError *err;is never NULLed, and ifg_file_get_contentsfails witherr == NULLcontrol falls through tog_strsplit (contents, ...)at:828withcontentsuninitialized. Fix: initialize both to NULL and return unconditionally on failure afterg_clear_error. - [High] Fail-open default is a Tizen regression given the missing packaging —
:811-815: file absent => allow everything. No package in api.git installs the.txt(packaging/machine-learning-api.spechas no entry), and nnstreamer's[element-restriction]ini section, still generated on Tizen, is no longer read by anyone after this diff. Net effect on Tizen: restriction silently turns off. Fix: land packaging in the same PR (list generated from the spec%define allowed_element_*macros, root-owned 0644), and on__TIZEN__treat a missing file as deny (or at least_ml_loge), keeping fail-open only for non-Tizen dev builds. - [Medium] Not thread-safe; reload storm on unreadable file —
:873-876:allowed_elementsand the two flag statics are mutated without a lock; two threads on first use cang_strfreevan array the other is iterating (UAF). When the file exists but is unreadable,is_element_restriction_validstays FALSE so the file is re-opened for every element of every construct. Fix: load once underg_once_init_enter/leave(or aGMutex), and make the unreadable case terminal (deny all, valid=TRUE). - [Medium] Brittle parsing —
:828-857:g_strsplit (contents, "\n")leaves\ron CRLF files, trailing whitespace is kept, whitespace-only lines count as elements,#is only honored at column 0, andfind_key_strvneeds an exact case-folded match so"appsrc\r"never matches. Fix:g_strstripeach line, skip empty /#-prefixed lines after stripping, accumulate into aGPtrArray; this also removes the two-pass count and the "changed at run-time" branch. - [Medium] Existing tests check a different config source than the library —
tests/capi/unittest_capi_inference.cc:2382still gateselement_available_01_ponnnsconf_get_custom_value_bool ("element-restriction", ...), which no longer relates to library behavior. Fix: tests that write a temp allow-list via an internal test-only setter (not an env var), and assert allowed/denied, empty file => deny all buttensor_*, comments/CRLF tolerated, file absent => allow all on non-Tizen. - [Low] Identifiers and style —
__load_allowed_elements/__is_element_allowed(:795,:873):__-prefixed identifiers are reserved in C and the file uses_ml_*; return type on the signature line andgchar *filepathviolate gst-indent (what cibot flagged). Typos: "alloed" (:810), "laod" (:845), "retriction" (title). - [Low] No documentation of the file contract — path, one-element-per-line format,
#comments, absent => allow all, empty => deny all. Add a short section inDocumentation/and a@remarksonml_check_element_availability. - [Low] Dependency goal only partially met — the function still relies on
find_key_strvfromnnstreamer_plugin_api.h, andunittest_capi_inference_single.ccstill callsnnsconf_get_custom_value_stringin seven places. A localg_strv_contains-style loop would dropfind_key_strvtoo.
No back-door found. The new code consults no environment variable or user-writable path; the only input is a root-owned /etc file. Compared with main it actually removes the NNSTREAMER_CONF env-influenced lookup, which is good; please keep it that way and use an internal test-only setter rather than an env override for tests.
Element restriction of Inference.Pipeline APIs should be done independently in api.git, not in nnstreamer.git
This addresses nnstreamer/nnstreamer#3553
The next commit should address
unit testing: unittest_capi_inference.cc nnstreamer_capi_util element_available_*
packaging (install the .txt file along with api rpms)
removing restriction mechanisms from nnstreamer.git