| Message ID | 20200404015624.30440-6-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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@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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@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@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; > > } > > > > /**
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@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@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
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; } /**
Create a helper class to handle cleanup of the mapped file to simplify error handling in loadIPAModuleInfo(). Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/libcamera/ipa_module.cpp | 58 +++++++++++++----------------------- 1 file changed, 21 insertions(+), 37 deletions(-)