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

[Niklas Söderlund]

Hi Laurent,

On 2020-04-08 02:07:29 +0300, Laurent Pinchart wrote:
> 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

I agree, I think we should only expose a surface related to cameras ;-) 
For qcam maybe their is something similar in Qt that can be leveraged.

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

-- 
Regards,
Niklas Söderlund