[libcamera-devel,RFC,03/10] libcamera: ipa_module: add loading error messages

1364 diff mbox series

Currently, if an IPAModule fails to be constructed due to size mismatch
of struct IPAModuleInfo, the error is simply "symbol not found". Add an
error message to say that the symbol is found, but not valid.

Also add an error message to tell, if an IPA module failed to load, the
path to the IPA module shared object that was attempted to be loaded.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
---
 src/libcamera/ipa_module.cpp | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Hi Paul,

Thank you for the patch.

On Wed, Jun 05, 2019 at 06:18:10PM -0400, Paul Elder wrote:
> Currently, if an IPAModule fails to be constructed due to size mismatch
> of struct IPAModuleInfo, the error is simply "symbol not found". Add an
> error message to say that the symbol is found, but not valid.
> 
> Also add an error message to tell, if an IPA module failed to load, the
> path to the IPA module shared object that was attempted to be loaded.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
>  src/libcamera/ipa_module.cpp | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp
> index e1d4b27..84c77f7 100644
> --- a/src/libcamera/ipa_module.cpp
> +++ b/src/libcamera/ipa_module.cpp
> @@ -145,6 +145,12 @@ int elfLoadSymbol(void *dst, size_t size, void *map, size_t soSize,
>  			targetSymbol = sym;
>  			break;
>  		}
> +
> +		if (!strcmp(name, symbol)) {
> +			LOG(IPAModule, Error)
> +				<< "Symbol " << symbol
> +				<< " found, but not valid. Check module version.";
> +		}

I'd rather not mention module version here, as elfLoadSymbol() is a
generic function, not specific to modules (we may reuse it later for
other ELF parsing needs). That's why I think it would be best for
elfLoadSymbol() to return both the pointer and the size, which can then
be checked in the caller with module-aware code. Furthermore, if we bump
the module info version later and need to implement backward
compatibility, it will be easier to load the symbol and then check for
the different supported sizes in the caller.

>  	}
>  
>  	if (targetSymbol == nullptr) {
> @@ -286,6 +292,12 @@ int IPAModule::loadIPAModuleInfo()
>  		ret = -EINVAL;
>  	}
>  
> +	if (ret)
> +		LOG(IPAModule, Error)
> +			<< "Error loading IPA module at "
> +			<< libPath_;
> +
> +
>  unmap:
>  	munmap(map, soSize);
>  close: