| Message ID | 20200319095406.GA26435@kaaira-HP-Pavilion-Notebook |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Kaaira, On 19/03/2020 09:54, 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 libcameraSourcePath() which > combines their functions, thereby making call sites simpler. > > Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in> > --- > src/libcamera/include/utils.h | 2 +- > src/libcamera/ipa_manager.cpp | 5 +++-- > src/libcamera/ipa_proxy.cpp | 6 +++--- > src/libcamera/utils.cpp | 20 +++++++++++--------- > 4 files changed, 18 insertions(+), 15 deletions(-) > > diff --git a/src/libcamera/include/utils.h b/src/libcamera/include/utils.h > index bc96e79..2cff15e 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 libcameraSourcePath(); > > } /* namespace utils */ > > diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp > index 0bd280c..cf9b6e7 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 libPath = utils::libcameraSourcePath(); > + if (!libPath.empty()) { > + std::string ipaBuildPath = libPath + "/../ipa"; I wonder if we should make utils::libcameraSourcePath return the root of the libcamera sources so that any users take a relative path from the root? That might make things clearer or more consistent as then the ipaBuildPath would become: std::string ipaBuildPath = root + "/src/ipa"; What do you think? if we do change to return the 'root', it would be a 'Build' directory root not a 'Source', so I think we might have to reconsider the name of the function too. > constexpr int maxDepth = 1; > > LOG(IPAManager, Info) > diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp > index 2f866cc..e9b7568 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 libPath = utils::libcameraSourcePath(); > + if (!libPath.empty()) { > + std::string ipaProxyDir = libPath + "/proxy/worker"; Of course, this would then become a bit longer: std::string ipaProxyDir = libPath + "src/libcamera/proxy/worker"; But I think it might make the intent clearer for both locations? > > 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..8212d3e 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 libcameraSourcePath() > { > - Dl_info info; > - > - /* Look up our own symbol. */ > - int ret = dladdr(reinterpret_cast<void *>(libcameraPath), &info); > - if (ret == 0) > - return nullptr; > - > - return info.dli_fname; > + if (!utils::isLibcameraInstalled()) { Because we are 'in' the utils:: namespace here, I don't think you need to explicitly state 'utils::' to call isLibcameraInstalled. The compiler will find the function without it. > + Dl_info info; > + /* Look up our own symbol. */ > + int ret = dladdr(reinterpret_cast<void *>(libcameraSourcePath), &info); > + if (ret == 0) > + return nullptr; > + > + return utils::dirname(info.dli_fname); If you drop the 'else' as stated below, you can invert this if, to if (ret) return utils::dirname(info.dli_fname); and let the error case fall through to the return std::string(); below which is better than returning a nullptr anyway, as the callers check this returned string for .empty() (so returning a nullptr is bad). > + } else > + return std::string(); You can drop the 'else', and put the return std::string(); as the last statement of the function. > } > > } /* namespace utils */ >
On Thu, Mar 19, 2020 at 10:36:14AM +0000, Kieran Bingham wrote: > Hi Kaaira, > > On 19/03/2020 09:54, 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 libcameraSourcePath() which > > combines their functions, thereby making call sites simpler. > > > > Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in> > > --- > > src/libcamera/include/utils.h | 2 +- > > src/libcamera/ipa_manager.cpp | 5 +++-- > > src/libcamera/ipa_proxy.cpp | 6 +++--- > > src/libcamera/utils.cpp | 20 +++++++++++--------- > > 4 files changed, 18 insertions(+), 15 deletions(-) > > > > diff --git a/src/libcamera/include/utils.h b/src/libcamera/include/utils.h > > index bc96e79..2cff15e 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 libcameraSourcePath(); > > > > } /* namespace utils */ > > > > diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp > > index 0bd280c..cf9b6e7 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 libPath = utils::libcameraSourcePath(); > > + if (!libPath.empty()) { > > + std::string ipaBuildPath = libPath + "/../ipa"; > > > I wonder if we should make utils::libcameraSourcePath return the root of > the libcamera sources so that any users take a relative path from the root? > > That might make things clearer or more consistent as then the > ipaBuildPath would become: Yes, that will make things more clean. I have updated the patch. > > std::string ipaBuildPath = root + "/src/ipa"; > > What do you think? > > if we do change to return the 'root', it would be a 'Build' directory > root not a 'Source', so I think we might have to reconsider the name of > the function too. > > > constexpr int maxDepth = 1; > > > > LOG(IPAManager, Info) > > diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp > > index 2f866cc..e9b7568 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 libPath = utils::libcameraSourcePath(); > > + if (!libPath.empty()) { > > + std::string ipaProxyDir = libPath + "/proxy/worker"; > > Of course, this would then become a bit longer: > > std::string ipaProxyDir = libPath + "src/libcamera/proxy/worker"; > > But I think it might make the intent clearer for both locations? > > > > > > 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..8212d3e 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 libcameraSourcePath() > > { > > - Dl_info info; > > - > > - /* Look up our own symbol. */ > > - int ret = dladdr(reinterpret_cast<void *>(libcameraPath), &info); > > - if (ret == 0) > > - return nullptr; > > - > > - return info.dli_fname; > > + if (!utils::isLibcameraInstalled()) { > > Because we are 'in' the utils:: namespace here, I don't think you need > to explicitly state 'utils::' to call isLibcameraInstalled. > > The compiler will find the function without it. > > > + Dl_info info; > > + /* Look up our own symbol. */ > > + int ret = dladdr(reinterpret_cast<void *>(libcameraSourcePath), &info); > > + if (ret == 0) > > + return nullptr; > > + > > + return utils::dirname(info.dli_fname); > > If you drop the 'else' as stated below, you can invert this if, to > > if (ret) > return utils::dirname(info.dli_fname); > > and let the error case fall through to the return std::string(); below > which is better than returning a nullptr anyway, as the callers check > this returned string for .empty() (so returning a nullptr is bad). > > > + } else > > + return std::string(); > > You can drop the 'else', and put the return std::string(); as the last > statement of the function. > > > > } > > > > } /* namespace utils */ > > > > -- > Regards > -- > Kieran
diff --git a/src/libcamera/include/utils.h b/src/libcamera/include/utils.h index bc96e79..2cff15e 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 libcameraSourcePath(); } /* namespace utils */ diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp index 0bd280c..cf9b6e7 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 libPath = utils::libcameraSourcePath(); + if (!libPath.empty()) { + std::string ipaBuildPath = libPath + "/../ipa"; constexpr int maxDepth = 1; LOG(IPAManager, Info) diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp index 2f866cc..e9b7568 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 libPath = utils::libcameraSourcePath(); + if (!libPath.empty()) { + std::string ipaProxyDir = libPath + "/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..8212d3e 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 libcameraSourcePath() { - Dl_info info; - - /* Look up our own symbol. */ - int ret = dladdr(reinterpret_cast<void *>(libcameraPath), &info); - if (ret == 0) - return nullptr; - - return info.dli_fname; + if (!utils::isLibcameraInstalled()) { + Dl_info info; + /* Look up our own symbol. */ + int ret = dladdr(reinterpret_cast<void *>(libcameraSourcePath), &info); + if (ret == 0) + return nullptr; + + return utils::dirname(info.dli_fname); + } else + 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 libcameraSourcePath() which combines their functions, thereby making call sites simpler. Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in> --- src/libcamera/include/utils.h | 2 +- src/libcamera/ipa_manager.cpp | 5 +++-- src/libcamera/ipa_proxy.cpp | 6 +++--- src/libcamera/utils.cpp | 20 +++++++++++--------- 4 files changed, 18 insertions(+), 15 deletions(-)