[libcamera-devel] [PATCH] libcamera: ipa_manager: Fix build without openssl

[Umang Jain]

Hi  Matthias, Kieran

On 9/14/22 1:44 PM, Kieran Bingham via libcamera-devel wrote:
> Quoting Matthias Fend via libcamera-devel (2022-09-14 08:00:25)
>> Since commit bedef55d9500 ("libcamera: pub_key: Gracefully handle failures
>> to load public key'") the build will fail if openssl is not found on the
>> host system.
>> Use the existing HAVE_IPA_PUBKEY define to avoid accessing pubKey_ which
>> does not exist when building without openssl.
>>
>> Signed-off-by: Matthias Fend <matthias.fend at emfend.at>
>> ---
>>   src/libcamera/ipa_manager.cpp | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
>> index 2f96a207..030ef43f 100644
>> --- a/src/libcamera/ipa_manager.cpp
>> +++ b/src/libcamera/ipa_manager.cpp
>> @@ -109,8 +109,10 @@ IPAManager::IPAManager()
>>                  LOG(IPAManager, Fatal)
>>                          << "Multiple IPAManager objects are not allowed";
>>   
>> +#if HAVE_IPA_PUBKEY
> If we have to add a #if here, it feels to me like the implementation
> for handling the key itself isn't right.
>
> The alternative is to construct an invalid public key at
> src/libcamera/ipa_pub_key.cpp.in to ensure we always have a key.. but
> I'm not sure that sounds a lot better either...
>
> This is limited to the IPA manager which knows about the keys anyway, so
> ... worst case I could see this as a solution ... but I think I would
> always want to push against ifdeffery spreading
>
> Could you check if it's reasonable to instantiate a pubKey_ which isn't
> valid please?
>
> But given here in ipa_manager - we already use #if HAVE_IPA_PUBKEY in
> IPAManager::isSignatureValid() I think I'd let this through.

We also have HAVE_IPA_PUBKEY #ifdeffery in the ipa_manager.h header 
which makes conditional existence for pubKey_ member

class IPAManager
{
...
private:
     #if HAVE_IPA_PUBKEY
             static const uint8_t publicKeyData_[];
             static const PubKey pubKey_;
     #endif
}

>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

With the above header hunk in mind, the patch looks fine to me for now,

Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>

>
>>          if (!pubKey_.isValid())
>>                  LOG(IPAManager, Warning) << "Public key not valid";
>> +#endif
>>   
>>          unsigned int ipaCount = 0;
>>   
>> -- 
>> 2.25.1
>>