[libcamera-devel] [PATCH 05/11] libcamera: ipa_module: Simplify error handling in loadIPAModuleInfo()
Niklas Söderlund
niklas.soderlund at ragnatech.se
Wed Apr 8 01:12:16 CEST 2020
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
More information about the libcamera-devel
mailing list