[libcamera-devel] [PATCH 05/11] libcamera: ipa_module: Simplify error handling in loadIPAModuleInfo()

[Niklas Söderlund]

Hi Laurent,

Thanks for your work.

On 2020-04-04 04:56:18 +0300, Laurent Pinchart wrote:
> Create a helper class to handle cleanup of the mapped file to simplify
> error handling in loadIPAModuleInfo().

s/Create/Use the File/

Wit this fixed,

Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>

I like some of the usage of tie{} and/or span<> in this patch, I think 
it could be used in cam/qcam to make the mapping of FileDescriptor(s) 
and their caching nicer.

> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  src/libcamera/ipa_module.cpp | 58 +++++++++++++-----------------------
>  1 file changed, 21 insertions(+), 37 deletions(-)
> 
> diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp
> index a01d0757ff8f..d1c411e14ba9 100644
> --- a/src/libcamera/ipa_module.cpp
> +++ b/src/libcamera/ipa_module.cpp
> @@ -21,6 +21,7 @@
>  #include <tuple>
>  #include <unistd.h>
>  
> +#include "file.h"
>  #include "log.h"
>  #include "pipeline_handler.h"
>  #include "utils.h"
> @@ -283,55 +284,38 @@ IPAModule::~IPAModule()
>  
>  int IPAModule::loadIPAModuleInfo()
>  {
> -	int fd = open(libPath_.c_str(), O_RDONLY);
> -	if (fd < 0) {
> -		int ret = -errno;
> +	File file{ libPath_ };
> +	if (!file.open(File::ReadOnly)) {
>  		LOG(IPAModule, Error) << "Failed to open IPA library: "
> -				      << strerror(-ret);
> -		return ret;
> +				      << strerror(-file.error());
> +		return file.error();
>  	}
>  
> -	void *data = nullptr;
> -	size_t dataSize;
> -	void *map;
> -	size_t soSize;
> -	struct stat st;
> -	int ret = fstat(fd, &st);
> -	if (ret < 0)
> -		goto close;
> -	soSize = st.st_size;
> -	map = mmap(NULL, soSize, PROT_READ, MAP_PRIVATE, fd, 0);
> -	if (map == MAP_FAILED) {
> -		ret = -errno;
> -		goto close;
> +	Span<uint8_t> data = file.map(0, -1, File::MapPrivate);
> +	int ret = elfVerifyIdent(data.data(), data.size());
> +	if (ret) {
> +		LOG(IPAModule, Error) << "IPA module is not an ELF file";
> +		return ret;
>  	}
>  
> -	ret = elfVerifyIdent(map, soSize);
> -	if (ret)
> -		goto unmap;
> +	void *info = nullptr;
> +	size_t infoSize;
>  
> -	std::tie(data, dataSize) = elfLoadSymbol(map, soSize, "ipaModuleInfo");
> -
> -	if (data && dataSize == sizeof(info_))
> -		memcpy(&info_, data, dataSize);
> +	std::tie(info, infoSize) = elfLoadSymbol(data.data(), data.size(),
> +						 "ipaModuleInfo");
> +	if (!info || infoSize != sizeof(info_)) {
> +		LOG(IPAModule, Error) << "IPA module has no valid info";
> +		return -EINVAL;
> +	}
>  
> -	if (!data)
> -		goto unmap;
> +	memcpy(&info_, info, infoSize);
>  
>  	if (info_.moduleAPIVersion != IPA_MODULE_API_VERSION) {
>  		LOG(IPAModule, Error) << "IPA module API version mismatch";
> -		ret = -EINVAL;
> +		return -EINVAL;
>  	}
>  
> -unmap:
> -	munmap(map, soSize);
> -close:
> -	if (ret || !data)
> -		LOG(IPAModule, Error)
> -			<< "Error loading IPA module info for " << libPath_;
> -
> -	close(fd);
> -	return ret;
> +	return 0;
>  }
>  
>  /**
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund