Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a build-time flag to disable SVP (Secure Video Path) support for environments that don't have SVP capabilities. The flag ENABLE_SVP defaults to ON, maintaining backward compatibility.
Changes:
- Added
ENABLE_SVPCMake option (defaults to ON) to control SVP support at build time - Guarded all SVP-related code with
#ifdef ENABLE_SVPdirectives - Provided stub implementations that return
SEC_RESULT_UNIMPLEMENTED_FEATUREwhen SVP is disabled - Updated test code to check for SVP capability support at runtime
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| CMakeLists.txt | Adds ENABLE_SVP build option (default ON) and sets corresponding compiler flags |
| include/sec_security_svp.h | Guards SVP-specific types, structs, and function declarations with ENABLE_SVP |
| src/sec_adapter_svp.c | Guards SVP implementation and adds stub functions returning unimplemented errors when disabled |
| src/sec_adapter_processor.c | Guards SVP buffer cleanup code and SVP-related command processing |
| src/sec_adapter_cipher.c | Guards all opaque buffer cipher operations to return unimplemented errors when SVP is disabled |
| test/openssl/src/test_creds_soc.cpp | Returns false for CAPABILITY_SVP when ENABLE_SVP is not defined |
| test/main/cpp/sec_api_utest_main.cpp | Wraps SVP-dependent test in runtime capability check |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mhabrat
left a comment
mhabrat
left a comment
There was a problem hiding this comment.
- Update and extend copyright years.
- Is there any reason to include sec_security_svp.h in any file? I'd add the ifdef around its inclusion.
| case SA_CRYPTO_RANDOM: | ||
| // Doesn't require a handle so called directly and not in the invoke thread. | ||
| case SA_GET_NAME: | ||
| case SA_SVP_BUFFER_ALLOC: |
There was a problem hiding this comment.
ok, tracing the code more, SA_SVP_BUFFER_ALLOC seems to have a buggy code path. It was ignored because the code doesn't do anything.
If you look at the code, using SA_SVP_BUFFER_ALLOC never even calls sa_svp_buffer_alloc, when maybe it should?
or was that removed for some purpose way back when?
| @@ -411,6 +411,10 @@ Sec_Result SecCipher_ProcessFragmented(Sec_CipherHandle* cipherHandle, SEC_BYTE* | |||
| */ | |||
| Sec_Result SecCipher_ProcessOpaque(Sec_CipherHandle* cipherHandle, Sec_OpaqueBufferHandle* inOpaqueBufferHandle, | |||
There was a problem hiding this comment.
Why include this function at all since it is specific to SVP?
There was a problem hiding this comment.
the function stub exists, but only returns an error message if someone tries to call it.
are you saying they should be removed completely? there is some value to keeping the stubs in responding with an unimplemented error, right? that way a caller would see that SVP isn't supported if called. although, maybe the better response is to just fail to link so they'll have a program crash and have to look at why?
| // Stub implementations when SVP is disabled | ||
|
|
||
| // Deprecated | ||
| Sec_Result Sec_OpaqueBufferMalloc(SEC_SIZE bufLength, void** handle, void* params) { |
There was a problem hiding this comment.
Why include these functions in the non-SVP code base? They are specific to SVP operation.
There was a problem hiding this comment.
I don't want to break the build if any app tries to call it, but I do want to return an error with a descriptive reason why.
2 participants
Issue #70: Disable SVP with build flag (it will still be enabled by default)