[libcamera-devel] [PATCH] libcamera: ipa_manager: Rework error messages when enumerating IPAs
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Sep 18 18:45:57 CEST 2019
Hi Kieran,
On Wed, Sep 18, 2019 at 02:31:58PM +0100, Kieran Bingham wrote:
> On 18/09/2019 14:14, Laurent Pinchart wrote:
> > On Tue, Sep 17, 2019 at 02:07:03AM -0400, Paul Elder wrote:
> >> On Sun, Sep 15, 2019 at 11:23:18PM +0300, Laurent Pinchart wrote:
> >>> When enumerating IPAs, the system IPA directory and all the directories
> >>> listed in the LIBCAMERA_IPA_MODULE_PATH environment variable are listed
> >>> in turn. Failing to list any of those directories results in an error
> >>> message being printed for every failure. This is particularly common
> >>> when developing libcamera, as IPAs may not have been installed locally.
> >>>
> >>> To avoid unnecessarily worrying error messages, rework the enumeration
> >>> procedure to only print a message when no IPA can be found at all.
> >>> Individual missing directories are not considered as an issue anymore.
> >>> The message is also downgraded from an error to a warning as the
> >>> situation may still be normal.
> >>>
> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >>> ---
> >>> src/libcamera/ipa_manager.cpp | 24 ++++++++++++++++++------
> >>> 1 file changed, 18 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
> >>> index 0a908eae6261..f3e08375820c 100644
> >>> --- a/src/libcamera/ipa_manager.cpp
> >>> +++ b/src/libcamera/ipa_manager.cpp
> >>> @@ -94,11 +94,19 @@ LOG_DEFINE_CATEGORY(IPAManager)
> >>>
> >>> IPAManager::IPAManager()
> >>> {
> >>> - addDir(IPA_MODULE_DIR);
> >>> + unsigned int ipaCount = 0;
> >>> + int ret;
> >>> +
> >>> + ret = addDir(IPA_MODULE_DIR);
> >>> + if (ret > 0)
> >>> + ipaCount += ret;
> >>> const char *modulePaths = utils::secure_getenv("LIBCAMERA_IPA_MODULE_PATH");
> >>> - if (!modulePaths)
> >>> + if (!modulePaths) {
> >>> + LOG(IPAManager, Warning)
> >>> + << "No IPA found in '" IPA_MODULE_DIR "'";
> >>
> >> We need to check ipaCount here, since we could have loaded some IPAs
> >> from the global dir, and just not have the environment variable
> >> specified.
> >
> > Oops, indeed. I'll fix this.
> >
> >>> return;
> >>> + }
> >>>
> >>> while (1) {
> >>> const char *delim = strchrnul(modulePaths, ':');
> >>> @@ -106,7 +114,9 @@ IPAManager::IPAManager()
> >>>
> >>> if (count) {
> >>> std::string path(modulePaths, count);
> >>> - addDir(path.c_str());
> >>> + ret = addDir(path.c_str());
> >>> + if (ret > 0)
> >>> + ipaCount += ret;
> >>> }
> >>>
> >>> if (*delim == '\0')
> >>> @@ -114,6 +124,11 @@ IPAManager::IPAManager()
> >>>
> >>> modulePaths += count + 1;
> >>> }
> >>> +
> >>> + if (!ipaCount)
> >>> + LOG(IPAManager, Warning)
> >>> + << "No IPA found in '" IPA_MODULE_DIR "' and '"
> >>> + << modulePaths << "'";
> >>
> >> I presume you're fine with modulePaths being printed as-is, as a
> >> colon-separated list of directories.
> >
> > I think it's good enough for now, yes.
> >
> >>> }
> >>>
> >>> IPAManager::~IPAManager()
> >>> @@ -155,9 +170,6 @@ int IPAManager::addDir(const char *libDir)
> >>> dir = opendir(libDir);
> >>> if (!dir) {
> >>> int ret = -errno;
> >>> - LOG(IPAManager, Error)
> >>> - << "Invalid path " << libDir << " for IPA modules: "
> >>> - << strerror(-ret);
> >>> return ret;
> >>
> >> Could this be shortened to
> >> return -errno; ?
> >
> > Unfortunately not, as LOG() could result in errno being modified. That's
> > why we have these constructs through the code where we store -errno in
> > ret.
>
> You mean the LOG() which you have removed right ;-)
This is embarassing... :-) I'll fix it when applying if there's no other
comment to v2, or in v3 otherwise.
> >>> }
> >>
> >> With these changes,
> >> Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list