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

17366 diff mbox series

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@emfend.at>
---
 src/libcamera/ipa_manager.cpp | 2 ++
 1 file changed, 2 insertions(+)

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@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.


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

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

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@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@ideasonboard.com>

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

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

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