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

[Laurent Pinchart]

Hi Niklas,

On Tue, Apr 07, 2020 at 10:27:35PM +0200, Niklas Söderlund wrote:
> 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/

Oops. This patch started with an internal helper class than I then
decided to split out to a File class.

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

We could even expose File in the public API, if it wasn't for the fear
of growing surface of the public API and having to keep binary
compatibility across releases :-S

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