| Message ID | 20200319124252.GA31289@kaaira-HP-Pavilion-Notebook |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Kaaira, On 19/03/2020 12:42, Kaaira Gupta wrote: > The two callers of functions libcameraPath() and isLibcameraInstalled() > end up using the same process and finally use the path with > libcamera.so. Hence write a function libcameraBuildPath() which > combines their functions and returns the root of the build sources > when the library has not been installed, but is running,thereby making > call sites simpler. > > When the library is imstalled, libcameraBuildPath() will return an empty /imstalled/installed/ If that's the only comment I'll fix while applying. > string. > > Make changes in the call sites accordingly. > > Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > Changes since v2: > - rename subject line and add necessary details in commit > message. > - Solve whitespace issues. > - specify the final 'slash' for directory path in return > statement itself. > > Changes since v1: > - rename function to libcameraBuildPath() instead, and return > the root of libcamera source, and not the source directory > root. > - Return empty string instead of nullptr when ret==0 > > src/libcamera/include/utils.h | 2 +- > src/libcamera/ipa_manager.cpp | 5 +++-- > src/libcamera/ipa_proxy.cpp | 6 +++--- > src/libcamera/utils.cpp | 16 +++++++++------- > 4 files changed, 16 insertions(+), 13 deletions(-) > > diff --git a/src/libcamera/include/utils.h b/src/libcamera/include/utils.h > index bc96e79..5dea8d2 100644 > --- a/src/libcamera/include/utils.h > +++ b/src/libcamera/include/utils.h > @@ -145,7 +145,7 @@ details::StringSplitter split(const std::string &str, const std::string &delim); > > bool isLibcameraInstalled(); > > -std::string libcameraPath(); > +std::string libcameraBuildPath(); > > } /* namespace utils */ > > diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp > index 0bd280c..bcaae35 100644 > --- a/src/libcamera/ipa_manager.cpp > +++ b/src/libcamera/ipa_manager.cpp > @@ -119,8 +119,9 @@ IPAManager::IPAManager() > * path for the IPA from that point. We need to recurse one level of > * sub-directories to match the build tree. > */ > - if (!utils::isLibcameraInstalled()) { > - std::string ipaBuildPath = utils::dirname(utils::libcameraPath()) + "/../ipa"; > + std::string root = utils::libcameraBuildPath(); > + if (!root.empty()) { > + std::string ipaBuildPath = root + "src/ipa"; > constexpr int maxDepth = 1; > > LOG(IPAManager, Info) > diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp > index 2f866cc..5fd88a4 100644 > --- a/src/libcamera/ipa_proxy.cpp > +++ b/src/libcamera/ipa_proxy.cpp > @@ -97,9 +97,9 @@ std::string IPAProxy::resolvePath(const std::string &file) const > * This requires identifying the path of the libcamera.so, and > * referencing a relative path for the proxy workers from that point. > */ > - if (!utils::isLibcameraInstalled()) { > - std::string ipaProxyDir = utils::dirname(utils::libcameraPath()) > - + "/proxy/worker"; > + std::string root = utils::libcameraBuildPath(); > + if (!root.empty()) { > + std::string ipaProxyDir = root + "src/libcamera/proxy/worker"; > > LOG(IPAProxy, Info) > << "libcamera is not installed. Loading proxy workers from'" > diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp > index 7e118fa..42c5c76 100644 > --- a/src/libcamera/utils.cpp > +++ b/src/libcamera/utils.cpp > @@ -347,16 +347,18 @@ bool isLibcameraInstalled() > * > * \return A string stating the path > */ > -std::string libcameraPath() > +std::string libcameraBuildPath() > { > - Dl_info info; > + if (!isLibcameraInstalled()) { > + Dl_info info; > > - /* Look up our own symbol. */ > - int ret = dladdr(reinterpret_cast<void *>(libcameraPath), &info); > - if (ret == 0) > - return nullptr; > + /* Look up our own symbol. */ > + int ret = dladdr(reinterpret_cast<void *>(libcameraBuildPath), &info); > + if (ret) > + return dirname(info.dli_fname) + "/../../"; > + } > > - return info.dli_fname; > + return std::string(); > } > > } /* namespace utils */ >
On Thu, Mar 19, 2020 at 12:52:01PM +0000, Kieran Bingham wrote: > Hi Kaaira, > > On 19/03/2020 12:42, Kaaira Gupta wrote: > > The two callers of functions libcameraPath() and isLibcameraInstalled() > > end up using the same process and finally use the path with > > libcamera.so. Hence write a function libcameraBuildPath() which > > combines their functions and returns the root of the build sources > > when the library has not been installed, but is running,thereby making > > call sites simpler. > > > > When the library is imstalled, libcameraBuildPath() will return an empty > > /imstalled/installed/ > > If that's the only comment I'll fix while applying. Do you check this manually or is there some tool which maybe I can also use from the next time so that there are no such errors? > > > string. > > > > Make changes in the call sites accordingly. > > > > Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > --- > > Changes since v2: > > - rename subject line and add necessary details in commit > > message. > > - Solve whitespace issues. > > - specify the final 'slash' for directory path in return > > statement itself. > > > > Changes since v1: > > - rename function to libcameraBuildPath() instead, and return > > the root of libcamera source, and not the source directory > > root. > > - Return empty string instead of nullptr when ret==0 > > > > src/libcamera/include/utils.h | 2 +- > > src/libcamera/ipa_manager.cpp | 5 +++-- > > src/libcamera/ipa_proxy.cpp | 6 +++--- > > src/libcamera/utils.cpp | 16 +++++++++------- > > 4 files changed, 16 insertions(+), 13 deletions(-) > > > > diff --git a/src/libcamera/include/utils.h b/src/libcamera/include/utils.h > > index bc96e79..5dea8d2 100644 > > --- a/src/libcamera/include/utils.h > > +++ b/src/libcamera/include/utils.h > > @@ -145,7 +145,7 @@ details::StringSplitter split(const std::string &str, const std::string &delim); > > > > bool isLibcameraInstalled(); > > > > -std::string libcameraPath(); > > +std::string libcameraBuildPath(); > > > > } /* namespace utils */ > > > > diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp > > index 0bd280c..bcaae35 100644 > > --- a/src/libcamera/ipa_manager.cpp > > +++ b/src/libcamera/ipa_manager.cpp > > @@ -119,8 +119,9 @@ IPAManager::IPAManager() > > * path for the IPA from that point. We need to recurse one level of > > * sub-directories to match the build tree. > > */ > > - if (!utils::isLibcameraInstalled()) { > > - std::string ipaBuildPath = utils::dirname(utils::libcameraPath()) + "/../ipa"; > > + std::string root = utils::libcameraBuildPath(); > > + if (!root.empty()) { > > + std::string ipaBuildPath = root + "src/ipa"; > > constexpr int maxDepth = 1; > > > > LOG(IPAManager, Info) > > diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp > > index 2f866cc..5fd88a4 100644 > > --- a/src/libcamera/ipa_proxy.cpp > > +++ b/src/libcamera/ipa_proxy.cpp > > @@ -97,9 +97,9 @@ std::string IPAProxy::resolvePath(const std::string &file) const > > * This requires identifying the path of the libcamera.so, and > > * referencing a relative path for the proxy workers from that point. > > */ > > - if (!utils::isLibcameraInstalled()) { > > - std::string ipaProxyDir = utils::dirname(utils::libcameraPath()) > > - + "/proxy/worker"; > > + std::string root = utils::libcameraBuildPath(); > > + if (!root.empty()) { > > + std::string ipaProxyDir = root + "src/libcamera/proxy/worker"; > > > > LOG(IPAProxy, Info) > > << "libcamera is not installed. Loading proxy workers from'" > > diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp > > index 7e118fa..42c5c76 100644 > > --- a/src/libcamera/utils.cpp > > +++ b/src/libcamera/utils.cpp > > @@ -347,16 +347,18 @@ bool isLibcameraInstalled() > > * > > * \return A string stating the path > > */ > > -std::string libcameraPath() > > +std::string libcameraBuildPath() > > { > > - Dl_info info; > > + if (!isLibcameraInstalled()) { > > + Dl_info info; > > > > - /* Look up our own symbol. */ > > - int ret = dladdr(reinterpret_cast<void *>(libcameraPath), &info); > > - if (ret == 0) > > - return nullptr; > > + /* Look up our own symbol. */ > > + int ret = dladdr(reinterpret_cast<void *>(libcameraBuildPath), &info); > > + if (ret) > > + return dirname(info.dli_fname) + "/../../"; > > + } > > > > - return info.dli_fname; > > + return std::string(); > > } > > > > } /* namespace utils */ > > > > -- > Regards > -- > Kieran
Hi Kaaira, Thank you for the patch. On Thu, Mar 19, 2020 at 06:12:52PM +0530, Kaaira Gupta wrote: > The two callers of functions libcameraPath() and isLibcameraInstalled() > end up using the same process and finally use the path with > libcamera.so. Hence write a function libcameraBuildPath() which > combines their functions and returns the root of the build sources > when the library has not been installed, but is running,thereby making s/running,/running, / We don't charge extra for white space :-) > call sites simpler. > > When the library is imstalled, libcameraBuildPath() will return an empty s/imstalled/installed/ > string. > > Make changes in the call sites accordingly. > > Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in> > --- > Changes since v2: > - rename subject line and add necessary details in commit > message. > - Solve whitespace issues. > - specify the final 'slash' for directory path in return > statement itself. > > Changes since v1: > - rename function to libcameraBuildPath() instead, and return > the root of libcamera source, and not the source directory > root. > - Return empty string instead of nullptr when ret==0 > > src/libcamera/include/utils.h | 2 +- > src/libcamera/ipa_manager.cpp | 5 +++-- > src/libcamera/ipa_proxy.cpp | 6 +++--- > src/libcamera/utils.cpp | 16 +++++++++------- > 4 files changed, 16 insertions(+), 13 deletions(-) > > diff --git a/src/libcamera/include/utils.h b/src/libcamera/include/utils.h > index bc96e79..5dea8d2 100644 > --- a/src/libcamera/include/utils.h > +++ b/src/libcamera/include/utils.h > @@ -145,7 +145,7 @@ details::StringSplitter split(const std::string &str, const std::string &delim); > > bool isLibcameraInstalled(); This function isn't used externally anymore, I would drop it from the header. > > -std::string libcameraPath(); > +std::string libcameraBuildPath(); > > } /* namespace utils */ > > diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp > index 0bd280c..bcaae35 100644 > --- a/src/libcamera/ipa_manager.cpp > +++ b/src/libcamera/ipa_manager.cpp > @@ -119,8 +119,9 @@ IPAManager::IPAManager() > * path for the IPA from that point. We need to recurse one level of > * sub-directories to match the build tree. > */ > - if (!utils::isLibcameraInstalled()) { > - std::string ipaBuildPath = utils::dirname(utils::libcameraPath()) + "/../ipa"; > + std::string root = utils::libcameraBuildPath(); > + if (!root.empty()) { > + std::string ipaBuildPath = root + "src/ipa"; Very neat :-) > constexpr int maxDepth = 1; > > LOG(IPAManager, Info) > diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp > index 2f866cc..5fd88a4 100644 > --- a/src/libcamera/ipa_proxy.cpp > +++ b/src/libcamera/ipa_proxy.cpp > @@ -97,9 +97,9 @@ std::string IPAProxy::resolvePath(const std::string &file) const > * This requires identifying the path of the libcamera.so, and > * referencing a relative path for the proxy workers from that point. > */ > - if (!utils::isLibcameraInstalled()) { > - std::string ipaProxyDir = utils::dirname(utils::libcameraPath()) > - + "/proxy/worker"; > + std::string root = utils::libcameraBuildPath(); > + if (!root.empty()) { > + std::string ipaProxyDir = root + "src/libcamera/proxy/worker"; > > LOG(IPAProxy, Info) > << "libcamera is not installed. Loading proxy workers from'" > diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp > index 7e118fa..42c5c76 100644 > --- a/src/libcamera/utils.cpp > +++ b/src/libcamera/utils.cpp > @@ -347,16 +347,18 @@ bool isLibcameraInstalled() > * > * \return A string stating the path > */ > -std::string libcameraPath() > +std::string libcameraBuildPath() > { > - Dl_info info; > + if (!isLibcameraInstalled()) { > + Dl_info info; > > - /* Look up our own symbol. */ > - int ret = dladdr(reinterpret_cast<void *>(libcameraPath), &info); > - if (ret == 0) > - return nullptr; > + /* Look up our own symbol. */ > + int ret = dladdr(reinterpret_cast<void *>(libcameraBuildPath), &info); > + if (ret) > + return dirname(info.dli_fname) + "/../../"; > + } > > - return info.dli_fname; > + return std::string(); Maybe it's my personal preference, but I find code easier to read when dealing away with errors immediately: if (isLibcameraInstalled()) return std::string(); Dl_info info; /* Look up our own symbol. */ int ret = dladdr(reinterpret_cast<void *>(libcameraBuildPath), &info); if (!ret) return std::string(); return dirname(info.dli_fname) + "/../../"; This way you can reduce indentation for most of the code that implements the real logic. If you're fine with this, I'm sure Kieran can change it while applying. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > } > > } /* namespace utils */
On 19/03/2020 13:02, Kaaira Gupta wrote: > On Thu, Mar 19, 2020 at 12:52:01PM +0000, Kieran Bingham wrote: >> Hi Kaaira, >> >> On 19/03/2020 12:42, Kaaira Gupta wrote: >>> The two callers of functions libcameraPath() and isLibcameraInstalled() >>> end up using the same process and finally use the path with >>> libcamera.so. Hence write a function libcameraBuildPath() which >>> combines their functions and returns the root of the build sources >>> when the library has not been installed, but is running,thereby making >>> call sites simpler. >>> >>> When the library is imstalled, libcameraBuildPath() will return an empty >> >> /imstalled/installed/ >> >> If that's the only comment I'll fix while applying. > > Do you check this manually or is there some tool which maybe I can also > use from the next time so that there are no such errors? In this instance it was my mail client highlighting spelling errors. I hope to be able to add spell-check support to checkstyle.py. It's something I've started, but haven't completed and the attempts I've made so far were really quite slow in performance :-( -- Kieran > >> >>> string. >>> >>> Make changes in the call sites accordingly. >>> >>> Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in> >> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> >>> --- >>> Changes since v2: >>> - rename subject line and add necessary details in commit >>> message. >>> - Solve whitespace issues. >>> - specify the final 'slash' for directory path in return >>> statement itself. >>> >>> Changes since v1: >>> - rename function to libcameraBuildPath() instead, and return >>> the root of libcamera source, and not the source directory >>> root. >>> - Return empty string instead of nullptr when ret==0 >>> >>> src/libcamera/include/utils.h | 2 +- >>> src/libcamera/ipa_manager.cpp | 5 +++-- >>> src/libcamera/ipa_proxy.cpp | 6 +++--- >>> src/libcamera/utils.cpp | 16 +++++++++------- >>> 4 files changed, 16 insertions(+), 13 deletions(-) >>> >>> diff --git a/src/libcamera/include/utils.h b/src/libcamera/include/utils.h >>> index bc96e79..5dea8d2 100644 >>> --- a/src/libcamera/include/utils.h >>> +++ b/src/libcamera/include/utils.h >>> @@ -145,7 +145,7 @@ details::StringSplitter split(const std::string &str, const std::string &delim); >>> >>> bool isLibcameraInstalled(); >>> >>> -std::string libcameraPath(); >>> +std::string libcameraBuildPath(); >>> >>> } /* namespace utils */ >>> >>> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp >>> index 0bd280c..bcaae35 100644 >>> --- a/src/libcamera/ipa_manager.cpp >>> +++ b/src/libcamera/ipa_manager.cpp >>> @@ -119,8 +119,9 @@ IPAManager::IPAManager() >>> * path for the IPA from that point. We need to recurse one level of >>> * sub-directories to match the build tree. >>> */ >>> - if (!utils::isLibcameraInstalled()) { >>> - std::string ipaBuildPath = utils::dirname(utils::libcameraPath()) + "/../ipa"; >>> + std::string root = utils::libcameraBuildPath(); >>> + if (!root.empty()) { >>> + std::string ipaBuildPath = root + "src/ipa"; >>> constexpr int maxDepth = 1; >>> >>> LOG(IPAManager, Info) >>> diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp >>> index 2f866cc..5fd88a4 100644 >>> --- a/src/libcamera/ipa_proxy.cpp >>> +++ b/src/libcamera/ipa_proxy.cpp >>> @@ -97,9 +97,9 @@ std::string IPAProxy::resolvePath(const std::string &file) const >>> * This requires identifying the path of the libcamera.so, and >>> * referencing a relative path for the proxy workers from that point. >>> */ >>> - if (!utils::isLibcameraInstalled()) { >>> - std::string ipaProxyDir = utils::dirname(utils::libcameraPath()) >>> - + "/proxy/worker"; >>> + std::string root = utils::libcameraBuildPath(); >>> + if (!root.empty()) { >>> + std::string ipaProxyDir = root + "src/libcamera/proxy/worker"; >>> >>> LOG(IPAProxy, Info) >>> << "libcamera is not installed. Loading proxy workers from'" >>> diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp >>> index 7e118fa..42c5c76 100644 >>> --- a/src/libcamera/utils.cpp >>> +++ b/src/libcamera/utils.cpp >>> @@ -347,16 +347,18 @@ bool isLibcameraInstalled() >>> * >>> * \return A string stating the path >>> */ >>> -std::string libcameraPath() >>> +std::string libcameraBuildPath() >>> { >>> - Dl_info info; >>> + if (!isLibcameraInstalled()) { >>> + Dl_info info; >>> >>> - /* Look up our own symbol. */ >>> - int ret = dladdr(reinterpret_cast<void *>(libcameraPath), &info); >>> - if (ret == 0) >>> - return nullptr; >>> + /* Look up our own symbol. */ >>> + int ret = dladdr(reinterpret_cast<void *>(libcameraBuildPath), &info); >>> + if (ret) >>> + return dirname(info.dli_fname) + "/../../"; >>> + } >>> >>> - return info.dli_fname; >>> + return std::string(); >>> } >>> >>> } /* namespace utils */ >>> >> >> -- >> Regards >> -- >> Kieran
Hi Laurent, Kaaira, On 19/03/2020 13:35, Laurent Pinchart wrote: > Hi Kaaira, > > Thank you for the patch. > > On Thu, Mar 19, 2020 at 06:12:52PM +0530, Kaaira Gupta wrote: >> The two callers of functions libcameraPath() and isLibcameraInstalled() >> end up using the same process and finally use the path with >> libcamera.so. Hence write a function libcameraBuildPath() which >> combines their functions and returns the root of the build sources >> when the library has not been installed, but is running,thereby making > > s/running,/running, / > > We don't charge extra for white space :-) Ooops I missed that one. > >> call sites simpler. >> >> When the library is imstalled, libcameraBuildPath() will return an empty > > s/imstalled/installed/ > >> string. >> >> Make changes in the call sites accordingly. >> >> Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in> >> --- >> Changes since v2: >> - rename subject line and add necessary details in commit >> message. >> - Solve whitespace issues. >> - specify the final 'slash' for directory path in return >> statement itself. >> >> Changes since v1: >> - rename function to libcameraBuildPath() instead, and return >> the root of libcamera source, and not the source directory >> root. >> - Return empty string instead of nullptr when ret==0 >> >> src/libcamera/include/utils.h | 2 +- >> src/libcamera/ipa_manager.cpp | 5 +++-- >> src/libcamera/ipa_proxy.cpp | 6 +++--- >> src/libcamera/utils.cpp | 16 +++++++++------- >> 4 files changed, 16 insertions(+), 13 deletions(-) >> >> diff --git a/src/libcamera/include/utils.h b/src/libcamera/include/utils.h >> index bc96e79..5dea8d2 100644 >> --- a/src/libcamera/include/utils.h >> +++ b/src/libcamera/include/utils.h >> @@ -145,7 +145,7 @@ details::StringSplitter split(const std::string &str, const std::string &delim); >> >> bool isLibcameraInstalled(); > > This function isn't used externally anymore, I would drop it from the > header. but it /could/ be used. It's not a static, and other locations might want to be able to detect this in the future. That was sort of the reason of keeping the two functions separate rather than merging them. Still it could easily be added back in if needed in the future too ... >> >> -std::string libcameraPath(); >> +std::string libcameraBuildPath(); >> >> } /* namespace utils */ >> >> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp >> index 0bd280c..bcaae35 100644 >> --- a/src/libcamera/ipa_manager.cpp >> +++ b/src/libcamera/ipa_manager.cpp >> @@ -119,8 +119,9 @@ IPAManager::IPAManager() >> * path for the IPA from that point. We need to recurse one level of >> * sub-directories to match the build tree. >> */ >> - if (!utils::isLibcameraInstalled()) { >> - std::string ipaBuildPath = utils::dirname(utils::libcameraPath()) + "/../ipa"; >> + std::string root = utils::libcameraBuildPath(); >> + if (!root.empty()) { >> + std::string ipaBuildPath = root + "src/ipa"; > > Very neat :-) > >> constexpr int maxDepth = 1; >> >> LOG(IPAManager, Info) >> diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp >> index 2f866cc..5fd88a4 100644 >> --- a/src/libcamera/ipa_proxy.cpp >> +++ b/src/libcamera/ipa_proxy.cpp >> @@ -97,9 +97,9 @@ std::string IPAProxy::resolvePath(const std::string &file) const >> * This requires identifying the path of the libcamera.so, and >> * referencing a relative path for the proxy workers from that point. >> */ >> - if (!utils::isLibcameraInstalled()) { >> - std::string ipaProxyDir = utils::dirname(utils::libcameraPath()) >> - + "/proxy/worker"; >> + std::string root = utils::libcameraBuildPath(); >> + if (!root.empty()) { >> + std::string ipaProxyDir = root + "src/libcamera/proxy/worker"; >> >> LOG(IPAProxy, Info) >> << "libcamera is not installed. Loading proxy workers from'" >> diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp >> index 7e118fa..42c5c76 100644 >> --- a/src/libcamera/utils.cpp >> +++ b/src/libcamera/utils.cpp >> @@ -347,16 +347,18 @@ bool isLibcameraInstalled() >> * >> * \return A string stating the path >> */ >> -std::string libcameraPath() >> +std::string libcameraBuildPath() >> { >> - Dl_info info; >> + if (!isLibcameraInstalled()) { >> + Dl_info info; >> >> - /* Look up our own symbol. */ >> - int ret = dladdr(reinterpret_cast<void *>(libcameraPath), &info); >> - if (ret == 0) >> - return nullptr; >> + /* Look up our own symbol. */ >> + int ret = dladdr(reinterpret_cast<void *>(libcameraBuildPath), &info); >> + if (ret) >> + return dirname(info.dli_fname) + "/../../"; >> + } >> >> - return info.dli_fname; >> + return std::string(); > > Maybe it's my personal preference, but I find code easier to read when > dealing away with errors immediately: > > if (isLibcameraInstalled()) > return std::string(); > > Dl_info info; > > /* Look up our own symbol. */ > int ret = dladdr(reinterpret_cast<void *>(libcameraBuildPath), &info); > if (!ret) > return std::string(); > > return dirname(info.dli_fname) + "/../../"; > > This way you can reduce indentation for most of the code that implements > the real logic. > > If you're fine with this, I'm sure Kieran can change it while applying. Hrm, I usually look to reduce the number of return paths ... but I'm not fussed either way. It's only because I had to work on MISRA projects in the past. Kaaira - up to you really I think. What do you prefer? If you like Laurent's version - please send a v4 with the other fixes and I can test and apply it. -- Kieran > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> } >> >> } /* namespace utils */ >
On Thu, Mar 19, 2020 at 02:05:23PM +0000, Kieran Bingham wrote: > Hi Laurent, Kaaira, > > On 19/03/2020 13:35, Laurent Pinchart wrote: > > Hi Kaaira, > > > > Thank you for the patch. > > > > On Thu, Mar 19, 2020 at 06:12:52PM +0530, Kaaira Gupta wrote: > >> The two callers of functions libcameraPath() and isLibcameraInstalled() > >> end up using the same process and finally use the path with > >> libcamera.so. Hence write a function libcameraBuildPath() which > >> combines their functions and returns the root of the build sources > >> when the library has not been installed, but is running,thereby making > > > > s/running,/running, / > > > > We don't charge extra for white space :-) > > Ooops I missed that one. > > > > >> call sites simpler. > >> > >> When the library is imstalled, libcameraBuildPath() will return an empty > > > > s/imstalled/installed/ > > > >> string. > >> > >> Make changes in the call sites accordingly. > >> > >> Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in> > >> --- > >> Changes since v2: > >> - rename subject line and add necessary details in commit > >> message. > >> - Solve whitespace issues. > >> - specify the final 'slash' for directory path in return > >> statement itself. > >> > >> Changes since v1: > >> - rename function to libcameraBuildPath() instead, and return > >> the root of libcamera source, and not the source directory > >> root. > >> - Return empty string instead of nullptr when ret==0 > >> > >> src/libcamera/include/utils.h | 2 +- > >> src/libcamera/ipa_manager.cpp | 5 +++-- > >> src/libcamera/ipa_proxy.cpp | 6 +++--- > >> src/libcamera/utils.cpp | 16 +++++++++------- > >> 4 files changed, 16 insertions(+), 13 deletions(-) > >> > >> diff --git a/src/libcamera/include/utils.h b/src/libcamera/include/utils.h > >> index bc96e79..5dea8d2 100644 > >> --- a/src/libcamera/include/utils.h > >> +++ b/src/libcamera/include/utils.h > >> @@ -145,7 +145,7 @@ details::StringSplitter split(const std::string &str, const std::string &delim); > >> > >> bool isLibcameraInstalled(); > > > > This function isn't used externally anymore, I would drop it from the > > header. > > but it /could/ be used. It's not a static, and other locations might > want to be able to detect this in the future. > > That was sort of the reason of keeping the two functions separate rather > than merging them. > > Still it could easily be added back in if needed in the future too ... Yes, we can drop it for now. I'll do that. > > >> > >> -std::string libcameraPath(); > >> +std::string libcameraBuildPath(); > >> > >> } /* namespace utils */ > >> > >> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp > >> index 0bd280c..bcaae35 100644 > >> --- a/src/libcamera/ipa_manager.cpp > >> +++ b/src/libcamera/ipa_manager.cpp > >> @@ -119,8 +119,9 @@ IPAManager::IPAManager() > >> * path for the IPA from that point. We need to recurse one level of > >> * sub-directories to match the build tree. > >> */ > >> - if (!utils::isLibcameraInstalled()) { > >> - std::string ipaBuildPath = utils::dirname(utils::libcameraPath()) + "/../ipa"; > >> + std::string root = utils::libcameraBuildPath(); > >> + if (!root.empty()) { > >> + std::string ipaBuildPath = root + "src/ipa"; > > > > Very neat :-) > > > >> constexpr int maxDepth = 1; > >> > >> LOG(IPAManager, Info) > >> diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp > >> index 2f866cc..5fd88a4 100644 > >> --- a/src/libcamera/ipa_proxy.cpp > >> +++ b/src/libcamera/ipa_proxy.cpp > >> @@ -97,9 +97,9 @@ std::string IPAProxy::resolvePath(const std::string &file) const > >> * This requires identifying the path of the libcamera.so, and > >> * referencing a relative path for the proxy workers from that point. > >> */ > >> - if (!utils::isLibcameraInstalled()) { > >> - std::string ipaProxyDir = utils::dirname(utils::libcameraPath()) > >> - + "/proxy/worker"; > >> + std::string root = utils::libcameraBuildPath(); > >> + if (!root.empty()) { > >> + std::string ipaProxyDir = root + "src/libcamera/proxy/worker"; > >> > >> LOG(IPAProxy, Info) > >> << "libcamera is not installed. Loading proxy workers from'" > >> diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp > >> index 7e118fa..42c5c76 100644 > >> --- a/src/libcamera/utils.cpp > >> +++ b/src/libcamera/utils.cpp > >> @@ -347,16 +347,18 @@ bool isLibcameraInstalled() > >> * > >> * \return A string stating the path > >> */ > >> -std::string libcameraPath() > >> +std::string libcameraBuildPath() > >> { > >> - Dl_info info; > >> + if (!isLibcameraInstalled()) { > >> + Dl_info info; > >> > >> - /* Look up our own symbol. */ > >> - int ret = dladdr(reinterpret_cast<void *>(libcameraPath), &info); > >> - if (ret == 0) > >> - return nullptr; > >> + /* Look up our own symbol. */ > >> + int ret = dladdr(reinterpret_cast<void *>(libcameraBuildPath), &info); > >> + if (ret) > >> + return dirname(info.dli_fname) + "/../../"; > >> + } > >> > >> - return info.dli_fname; > >> + return std::string(); > > > > Maybe it's my personal preference, but I find code easier to read when > > dealing away with errors immediately: > > > > if (isLibcameraInstalled()) > > return std::string(); > > > > Dl_info info; > > > > /* Look up our own symbol. */ > > int ret = dladdr(reinterpret_cast<void *>(libcameraBuildPath), &info); > > if (!ret) > > return std::string(); > > > > return dirname(info.dli_fname) + "/../../"; > > > > This way you can reduce indentation for most of the code that implements > > the real logic. > > > > If you're fine with this, I'm sure Kieran can change it while applying. > > Hrm, I usually look to reduce the number of return paths ... but I'm not > fussed either way. It's only because I had to work on MISRA projects in > the past. > > Kaaira - up to you really I think. What do you prefer? If you like > Laurent's version - please send a v4 with the other fixes and I can test > and apply it. I think I should change it, that way the code looks cleaner. I'll send the updated version. > > -- > Kieran > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > >> } > >> > >> } /* namespace utils */ > >
Hi Kieran, On Thu, Mar 19, 2020 at 02:05:23PM +0000, Kieran Bingham wrote: > On 19/03/2020 13:35, Laurent Pinchart wrote: > > On Thu, Mar 19, 2020 at 06:12:52PM +0530, Kaaira Gupta wrote: > >> The two callers of functions libcameraPath() and isLibcameraInstalled() > >> end up using the same process and finally use the path with > >> libcamera.so. Hence write a function libcameraBuildPath() which > >> combines their functions and returns the root of the build sources > >> when the library has not been installed, but is running,thereby making > > > > s/running,/running, / > > > > We don't charge extra for white space :-) > > Ooops I missed that one. > > >> call sites simpler. > >> > >> When the library is imstalled, libcameraBuildPath() will return an empty > > > > s/imstalled/installed/ > > > >> string. > >> > >> Make changes in the call sites accordingly. > >> > >> Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in> > >> --- > >> Changes since v2: > >> - rename subject line and add necessary details in commit > >> message. > >> - Solve whitespace issues. > >> - specify the final 'slash' for directory path in return > >> statement itself. > >> > >> Changes since v1: > >> - rename function to libcameraBuildPath() instead, and return > >> the root of libcamera source, and not the source directory > >> root. > >> - Return empty string instead of nullptr when ret==0 > >> > >> src/libcamera/include/utils.h | 2 +- > >> src/libcamera/ipa_manager.cpp | 5 +++-- > >> src/libcamera/ipa_proxy.cpp | 6 +++--- > >> src/libcamera/utils.cpp | 16 +++++++++------- > >> 4 files changed, 16 insertions(+), 13 deletions(-) > >> > >> diff --git a/src/libcamera/include/utils.h b/src/libcamera/include/utils.h > >> index bc96e79..5dea8d2 100644 > >> --- a/src/libcamera/include/utils.h > >> +++ b/src/libcamera/include/utils.h > >> @@ -145,7 +145,7 @@ details::StringSplitter split(const std::string &str, const std::string &delim); > >> > >> bool isLibcameraInstalled(); > > > > This function isn't used externally anymore, I would drop it from the > > header. > > but it /could/ be used. It's not a static, and other locations might > want to be able to detect this in the future. > > That was sort of the reason of keeping the two functions separate rather > than merging them. I think I'd rather not have callers check if libcamera is installed, but instead go for a bit of a higher level API where callers get the information they need, such as the build root directory. > Still it could easily be added back in if needed in the future too ... That too, yes. > >> > >> -std::string libcameraPath(); > >> +std::string libcameraBuildPath(); > >> > >> } /* namespace utils */ > >> > >> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp > >> index 0bd280c..bcaae35 100644 > >> --- a/src/libcamera/ipa_manager.cpp > >> +++ b/src/libcamera/ipa_manager.cpp > >> @@ -119,8 +119,9 @@ IPAManager::IPAManager() > >> * path for the IPA from that point. We need to recurse one level of > >> * sub-directories to match the build tree. > >> */ > >> - if (!utils::isLibcameraInstalled()) { > >> - std::string ipaBuildPath = utils::dirname(utils::libcameraPath()) + "/../ipa"; > >> + std::string root = utils::libcameraBuildPath(); > >> + if (!root.empty()) { > >> + std::string ipaBuildPath = root + "src/ipa"; > > > > Very neat :-) > > > >> constexpr int maxDepth = 1; > >> > >> LOG(IPAManager, Info) > >> diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp > >> index 2f866cc..5fd88a4 100644 > >> --- a/src/libcamera/ipa_proxy.cpp > >> +++ b/src/libcamera/ipa_proxy.cpp > >> @@ -97,9 +97,9 @@ std::string IPAProxy::resolvePath(const std::string &file) const > >> * This requires identifying the path of the libcamera.so, and > >> * referencing a relative path for the proxy workers from that point. > >> */ > >> - if (!utils::isLibcameraInstalled()) { > >> - std::string ipaProxyDir = utils::dirname(utils::libcameraPath()) > >> - + "/proxy/worker"; > >> + std::string root = utils::libcameraBuildPath(); > >> + if (!root.empty()) { > >> + std::string ipaProxyDir = root + "src/libcamera/proxy/worker"; > >> > >> LOG(IPAProxy, Info) > >> << "libcamera is not installed. Loading proxy workers from'" > >> diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp > >> index 7e118fa..42c5c76 100644 > >> --- a/src/libcamera/utils.cpp > >> +++ b/src/libcamera/utils.cpp > >> @@ -347,16 +347,18 @@ bool isLibcameraInstalled() > >> * > >> * \return A string stating the path > >> */ > >> -std::string libcameraPath() > >> +std::string libcameraBuildPath() > >> { > >> - Dl_info info; > >> + if (!isLibcameraInstalled()) { > >> + Dl_info info; > >> > >> - /* Look up our own symbol. */ > >> - int ret = dladdr(reinterpret_cast<void *>(libcameraPath), &info); > >> - if (ret == 0) > >> - return nullptr; > >> + /* Look up our own symbol. */ > >> + int ret = dladdr(reinterpret_cast<void *>(libcameraBuildPath), &info); > >> + if (ret) > >> + return dirname(info.dli_fname) + "/../../"; > >> + } > >> > >> - return info.dli_fname; > >> + return std::string(); > > > > Maybe it's my personal preference, but I find code easier to read when > > dealing away with errors immediately: > > > > if (isLibcameraInstalled()) > > return std::string(); > > > > Dl_info info; > > > > /* Look up our own symbol. */ > > int ret = dladdr(reinterpret_cast<void *>(libcameraBuildPath), &info); > > if (!ret) > > return std::string(); > > > > return dirname(info.dli_fname) + "/../../"; > > > > This way you can reduce indentation for most of the code that implements > > the real logic. > > > > If you're fine with this, I'm sure Kieran can change it while applying. > > Hrm, I usually look to reduce the number of return paths ... but I'm not > fussed either way. It's only because I had to work on MISRA projects in > the past. So less return paths are considered to lead to less bugs ? Interesting concept, but I'm not sure how true it is :-) > Kaaira - up to you really I think. What do you prefer? If you like > Laurent's version - please send a v4 with the other fixes and I can test > and apply it. > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > >> } > >> > >> } /* namespace utils */
On Thu, Mar 19, 2020 at 01:36:53PM +0000, Kieran Bingham wrote: > On 19/03/2020 13:02, Kaaira Gupta wrote: > > On Thu, Mar 19, 2020 at 12:52:01PM +0000, Kieran Bingham wrote: > >> Hi Kaaira, > >> > >> On 19/03/2020 12:42, Kaaira Gupta wrote: > >>> The two callers of functions libcameraPath() and isLibcameraInstalled() > >>> end up using the same process and finally use the path with > >>> libcamera.so. Hence write a function libcameraBuildPath() which > >>> combines their functions and returns the root of the build sources > >>> when the library has not been installed, but is running,thereby making > >>> call sites simpler. > >>> > >>> When the library is imstalled, libcameraBuildPath() will return an empty > >> > >> /imstalled/installed/ > >> > >> If that's the only comment I'll fix while applying. > > > > Do you check this manually or is there some tool which maybe I can also > > use from the next time so that there are no such errors? > > In this instance it was my mail client highlighting spelling errors. > > I hope to be able to add spell-check support to checkstyle.py. It's > something I've started, but haven't completed and the attempts I've made > so far were really quite slow in performance :-( Can't the checkpatch.pl script used in kernel be used for this as well? It almost checks everything > > -- > Kieran > > > > > >> > >>> string. > >>> > >>> Make changes in the call sites accordingly. > >>> > >>> Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in> > >> > >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> > >>> --- > >>> Changes since v2: > >>> - rename subject line and add necessary details in commit > >>> message. > >>> - Solve whitespace issues. > >>> - specify the final 'slash' for directory path in return > >>> statement itself. > >>> > >>> Changes since v1: > >>> - rename function to libcameraBuildPath() instead, and return > >>> the root of libcamera source, and not the source directory > >>> root. > >>> - Return empty string instead of nullptr when ret==0 > >>> > >>> src/libcamera/include/utils.h | 2 +- > >>> src/libcamera/ipa_manager.cpp | 5 +++-- > >>> src/libcamera/ipa_proxy.cpp | 6 +++--- > >>> src/libcamera/utils.cpp | 16 +++++++++------- > >>> 4 files changed, 16 insertions(+), 13 deletions(-) > >>> > >>> diff --git a/src/libcamera/include/utils.h b/src/libcamera/include/utils.h > >>> index bc96e79..5dea8d2 100644 > >>> --- a/src/libcamera/include/utils.h > >>> +++ b/src/libcamera/include/utils.h > >>> @@ -145,7 +145,7 @@ details::StringSplitter split(const std::string &str, const std::string &delim); > >>> > >>> bool isLibcameraInstalled(); > >>> > >>> -std::string libcameraPath(); > >>> +std::string libcameraBuildPath(); > >>> > >>> } /* namespace utils */ > >>> > >>> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp > >>> index 0bd280c..bcaae35 100644 > >>> --- a/src/libcamera/ipa_manager.cpp > >>> +++ b/src/libcamera/ipa_manager.cpp > >>> @@ -119,8 +119,9 @@ IPAManager::IPAManager() > >>> * path for the IPA from that point. We need to recurse one level of > >>> * sub-directories to match the build tree. > >>> */ > >>> - if (!utils::isLibcameraInstalled()) { > >>> - std::string ipaBuildPath = utils::dirname(utils::libcameraPath()) + "/../ipa"; > >>> + std::string root = utils::libcameraBuildPath(); > >>> + if (!root.empty()) { > >>> + std::string ipaBuildPath = root + "src/ipa"; > >>> constexpr int maxDepth = 1; > >>> > >>> LOG(IPAManager, Info) > >>> diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp > >>> index 2f866cc..5fd88a4 100644 > >>> --- a/src/libcamera/ipa_proxy.cpp > >>> +++ b/src/libcamera/ipa_proxy.cpp > >>> @@ -97,9 +97,9 @@ std::string IPAProxy::resolvePath(const std::string &file) const > >>> * This requires identifying the path of the libcamera.so, and > >>> * referencing a relative path for the proxy workers from that point. > >>> */ > >>> - if (!utils::isLibcameraInstalled()) { > >>> - std::string ipaProxyDir = utils::dirname(utils::libcameraPath()) > >>> - + "/proxy/worker"; > >>> + std::string root = utils::libcameraBuildPath(); > >>> + if (!root.empty()) { > >>> + std::string ipaProxyDir = root + "src/libcamera/proxy/worker"; > >>> > >>> LOG(IPAProxy, Info) > >>> << "libcamera is not installed. Loading proxy workers from'" > >>> diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp > >>> index 7e118fa..42c5c76 100644 > >>> --- a/src/libcamera/utils.cpp > >>> +++ b/src/libcamera/utils.cpp > >>> @@ -347,16 +347,18 @@ bool isLibcameraInstalled() > >>> * > >>> * \return A string stating the path > >>> */ > >>> -std::string libcameraPath() > >>> +std::string libcameraBuildPath() > >>> { > >>> - Dl_info info; > >>> + if (!isLibcameraInstalled()) { > >>> + Dl_info info; > >>> > >>> - /* Look up our own symbol. */ > >>> - int ret = dladdr(reinterpret_cast<void *>(libcameraPath), &info); > >>> - if (ret == 0) > >>> - return nullptr; > >>> + /* Look up our own symbol. */ > >>> + int ret = dladdr(reinterpret_cast<void *>(libcameraBuildPath), &info); > >>> + if (ret) > >>> + return dirname(info.dli_fname) + "/../../"; > >>> + } > >>> > >>> - return info.dli_fname; > >>> + return std::string(); > >>> } > >>> > >>> } /* namespace utils */ > >>> > >> > >> -- > >> Regards > >> -- > >> Kieran > > -- > Regards > -- > Kieran
diff --git a/src/libcamera/include/utils.h b/src/libcamera/include/utils.h index bc96e79..5dea8d2 100644 --- a/src/libcamera/include/utils.h +++ b/src/libcamera/include/utils.h @@ -145,7 +145,7 @@ details::StringSplitter split(const std::string &str, const std::string &delim); bool isLibcameraInstalled(); -std::string libcameraPath(); +std::string libcameraBuildPath(); } /* namespace utils */ diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp index 0bd280c..bcaae35 100644 --- a/src/libcamera/ipa_manager.cpp +++ b/src/libcamera/ipa_manager.cpp @@ -119,8 +119,9 @@ IPAManager::IPAManager() * path for the IPA from that point. We need to recurse one level of * sub-directories to match the build tree. */ - if (!utils::isLibcameraInstalled()) { - std::string ipaBuildPath = utils::dirname(utils::libcameraPath()) + "/../ipa"; + std::string root = utils::libcameraBuildPath(); + if (!root.empty()) { + std::string ipaBuildPath = root + "src/ipa"; constexpr int maxDepth = 1; LOG(IPAManager, Info) diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp index 2f866cc..5fd88a4 100644 --- a/src/libcamera/ipa_proxy.cpp +++ b/src/libcamera/ipa_proxy.cpp @@ -97,9 +97,9 @@ std::string IPAProxy::resolvePath(const std::string &file) const * This requires identifying the path of the libcamera.so, and * referencing a relative path for the proxy workers from that point. */ - if (!utils::isLibcameraInstalled()) { - std::string ipaProxyDir = utils::dirname(utils::libcameraPath()) - + "/proxy/worker"; + std::string root = utils::libcameraBuildPath(); + if (!root.empty()) { + std::string ipaProxyDir = root + "src/libcamera/proxy/worker"; LOG(IPAProxy, Info) << "libcamera is not installed. Loading proxy workers from'" diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp index 7e118fa..42c5c76 100644 --- a/src/libcamera/utils.cpp +++ b/src/libcamera/utils.cpp @@ -347,16 +347,18 @@ bool isLibcameraInstalled() * * \return A string stating the path */ -std::string libcameraPath() +std::string libcameraBuildPath() { - Dl_info info; + if (!isLibcameraInstalled()) { + Dl_info info; - /* Look up our own symbol. */ - int ret = dladdr(reinterpret_cast<void *>(libcameraPath), &info); - if (ret == 0) - return nullptr; + /* Look up our own symbol. */ + int ret = dladdr(reinterpret_cast<void *>(libcameraBuildPath), &info); + if (ret) + return dirname(info.dli_fname) + "/../../"; + } - return info.dli_fname; + return std::string(); } } /* namespace utils */
The two callers of functions libcameraPath() and isLibcameraInstalled() end up using the same process and finally use the path with libcamera.so. Hence write a function libcameraBuildPath() which combines their functions and returns the root of the build sources when the library has not been installed, but is running,thereby making call sites simpler. When the library is imstalled, libcameraBuildPath() will return an empty string. Make changes in the call sites accordingly. Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in> --- Changes since v2: - rename subject line and add necessary details in commit message. - Solve whitespace issues. - specify the final 'slash' for directory path in return statement itself. Changes since v1: - rename function to libcameraBuildPath() instead, and return the root of libcamera source, and not the source directory root. - Return empty string instead of nullptr when ret==0 src/libcamera/include/utils.h | 2 +- src/libcamera/ipa_manager.cpp | 5 +++-- src/libcamera/ipa_proxy.cpp | 6 +++--- src/libcamera/utils.cpp | 16 +++++++++------- 4 files changed, 16 insertions(+), 13 deletions(-)