| Message ID | 20230427071652.25828-1-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush On Thu, Apr 27, 2023 at 08:16:52AM +0100, Naushir Patuck via libcamera-devel wrote: > Add a prefix parameter to IPAProxy::configurationFile(). This prefix is > added to the search path when locating IPA configuration files in the > system directories. > > For example, the system directories etc/libcamera/ipa/<prefix>/ and > share/libcamera/ipa/<prefix>/ will be used to search for the IPA > configuration files. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > include/libcamera/internal/ipa_proxy.h | 3 ++- > src/libcamera/ipa_proxy.cpp | 11 +++++++---- > 2 files changed, 9 insertions(+), 5 deletions(-) > > diff --git a/include/libcamera/internal/ipa_proxy.h b/include/libcamera/internal/ipa_proxy.h > index 781c8b623605..9235b582edad 100644 > --- a/include/libcamera/internal/ipa_proxy.h > +++ b/include/libcamera/internal/ipa_proxy.h > @@ -31,7 +31,8 @@ public: > > bool isValid() const { return valid_; } > > - std::string configurationFile(const std::string &file) const; > + std::string configurationFile(const std::string &file, > + const std::string &prefix = "") const; > > protected: > std::string resolvePath(const std::string &file) const; > diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp > index 3f2cc6b89f60..4a27b0a993fa 100644 > --- a/src/libcamera/ipa_proxy.cpp > +++ b/src/libcamera/ipa_proxy.cpp > @@ -72,6 +72,7 @@ IPAProxy::~IPAProxy() > /** > * \brief Retrieve the absolute path to an IPA configuration file > * \param[in] name The configuration file name > + * \param[in] prefix The configuration directory prefix when searching system paths > * > * This function locates the configuration file for an IPA and returns its > * absolute path. It searches the following directories, in order: > @@ -80,8 +81,8 @@ IPAProxy::~IPAProxy() > * environment variable ; or > * - If libcamera is not installed, the src/ipa/ directory within the source > * tree ; otherwise > - * - The system sysconf (etc/libcamera/ipa) and the data (share/libcamera/ipa/) > - * directories. > + * - The system sysconf (etc/libcamera/ipa/<prefix>/) and the data > + * (share/libcamera/ipa/<prefix>/) directories. Doxygen doesn't seem happy with <prefix> src/libcamera/ipa_proxy.cpp:83: warning: Unsupported xml/html tag <prefix> found src/libcamera/ipa_proxy.cpp:84: warning: Unsupported xml/html tag <prefix> found > * > * The system directories are not searched if libcamera is not installed. > * > @@ -92,7 +93,8 @@ IPAProxy::~IPAProxy() > * \return The full path to the IPA configuration file, or an empty string if > * no configuration file can be found > */ > -std::string IPAProxy::configurationFile(const std::string &name) const > +std::string IPAProxy::configurationFile(const std::string &name, > + const std::string &prefix) const > { > struct stat statbuf; > int ret; > @@ -139,7 +141,8 @@ std::string IPAProxy::configurationFile(const std::string &name) const > } else { > /* Else look in the system locations. */ > for (const auto &dir : utils::split(IPA_CONFIG_DIR, ":")) { > - std::string confPath = dir + "/" + ipaName + "/" + name; > + std::string confPath = dir + "/" + prefix + "/" + > + ipaName + "/" + name; > ret = stat(confPath.c_str(), &statbuf); > if (ret == 0 && (statbuf.st_mode & S_IFMT) == S_IFREG) > return confPath; > -- > 2.34.1 >
On Thu, 27 Apr 2023 at 09:10, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Hi Naush > > On Thu, Apr 27, 2023 at 08:16:52AM +0100, Naushir Patuck via libcamera-devel wrote: > > Add a prefix parameter to IPAProxy::configurationFile(). This prefix is > > added to the search path when locating IPA configuration files in the > > system directories. > > > > For example, the system directories etc/libcamera/ipa/<prefix>/ and > > share/libcamera/ipa/<prefix>/ will be used to search for the IPA > > configuration files. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > include/libcamera/internal/ipa_proxy.h | 3 ++- > > src/libcamera/ipa_proxy.cpp | 11 +++++++---- > > 2 files changed, 9 insertions(+), 5 deletions(-) > > > > diff --git a/include/libcamera/internal/ipa_proxy.h b/include/libcamera/internal/ipa_proxy.h > > index 781c8b623605..9235b582edad 100644 > > --- a/include/libcamera/internal/ipa_proxy.h > > +++ b/include/libcamera/internal/ipa_proxy.h > > @@ -31,7 +31,8 @@ public: > > > > bool isValid() const { return valid_; } > > > > - std::string configurationFile(const std::string &file) const; > > + std::string configurationFile(const std::string &file, > > + const std::string &prefix = "") const; > > > > protected: > > std::string resolvePath(const std::string &file) const; > > diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp > > index 3f2cc6b89f60..4a27b0a993fa 100644 > > --- a/src/libcamera/ipa_proxy.cpp > > +++ b/src/libcamera/ipa_proxy.cpp > > @@ -72,6 +72,7 @@ IPAProxy::~IPAProxy() > > /** > > * \brief Retrieve the absolute path to an IPA configuration file > > * \param[in] name The configuration file name > > + * \param[in] prefix The configuration directory prefix when searching system paths > > * > > * This function locates the configuration file for an IPA and returns its > > * absolute path. It searches the following directories, in order: > > @@ -80,8 +81,8 @@ IPAProxy::~IPAProxy() > > * environment variable ; or > > * - If libcamera is not installed, the src/ipa/ directory within the source > > * tree ; otherwise > > - * - The system sysconf (etc/libcamera/ipa) and the data (share/libcamera/ipa/) > > - * directories. > > + * - The system sysconf (etc/libcamera/ipa/<prefix>/) and the data > > + * (share/libcamera/ipa/<prefix>/) directories. > > Doxygen doesn't seem happy with <prefix> > > src/libcamera/ipa_proxy.cpp:83: warning: Unsupported xml/html tag <prefix> found > src/libcamera/ipa_proxy.cpp:84: warning: Unsupported xml/html tag <prefix> found Oops! Should I just use "prefix" instead? "" The system sysconf (etc/libcamera/ipa/prefix/) and the data (share/libcamera/ipa/prefix/) directories. "" Regards, Naush > > > > * > > * The system directories are not searched if libcamera is not installed. > > * > > @@ -92,7 +93,8 @@ IPAProxy::~IPAProxy() > > * \return The full path to the IPA configuration file, or an empty string if > > * no configuration file can be found > > */ > > -std::string IPAProxy::configurationFile(const std::string &name) const > > +std::string IPAProxy::configurationFile(const std::string &name, > > + const std::string &prefix) const > > { > > struct stat statbuf; > > int ret; > > @@ -139,7 +141,8 @@ std::string IPAProxy::configurationFile(const std::string &name) const > > } else { > > /* Else look in the system locations. */ > > for (const auto &dir : utils::split(IPA_CONFIG_DIR, ":")) { > > - std::string confPath = dir + "/" + ipaName + "/" + name; > > + std::string confPath = dir + "/" + prefix + "/" + > > + ipaName + "/" + name; > > ret = stat(confPath.c_str(), &statbuf); > > if (ret == 0 && (statbuf.st_mode & S_IFMT) == S_IFREG) > > return confPath; > > -- > > 2.34.1 > >
Naush, what would be the implications of setting the IPA module name to "rpi/vc4" ? That would make this patch not needed, right ? Are there undesired implications ? On Thu, Apr 27, 2023 at 09:24:16AM +0100, Naushir Patuck wrote: > On Thu, 27 Apr 2023 at 09:10, Jacopo Mondi > <jacopo.mondi@ideasonboard.com> wrote: > > > > Hi Naush > > > > On Thu, Apr 27, 2023 at 08:16:52AM +0100, Naushir Patuck via libcamera-devel wrote: > > > Add a prefix parameter to IPAProxy::configurationFile(). This prefix is > > > added to the search path when locating IPA configuration files in the > > > system directories. > > > > > > For example, the system directories etc/libcamera/ipa/<prefix>/ and > > > share/libcamera/ipa/<prefix>/ will be used to search for the IPA > > > configuration files. > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > --- > > > include/libcamera/internal/ipa_proxy.h | 3 ++- > > > src/libcamera/ipa_proxy.cpp | 11 +++++++---- > > > 2 files changed, 9 insertions(+), 5 deletions(-) > > > > > > diff --git a/include/libcamera/internal/ipa_proxy.h b/include/libcamera/internal/ipa_proxy.h > > > index 781c8b623605..9235b582edad 100644 > > > --- a/include/libcamera/internal/ipa_proxy.h > > > +++ b/include/libcamera/internal/ipa_proxy.h > > > @@ -31,7 +31,8 @@ public: > > > > > > bool isValid() const { return valid_; } > > > > > > - std::string configurationFile(const std::string &file) const; > > > + std::string configurationFile(const std::string &file, > > > + const std::string &prefix = "") const; > > > > > > protected: > > > std::string resolvePath(const std::string &file) const; > > > diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp > > > index 3f2cc6b89f60..4a27b0a993fa 100644 > > > --- a/src/libcamera/ipa_proxy.cpp > > > +++ b/src/libcamera/ipa_proxy.cpp > > > @@ -72,6 +72,7 @@ IPAProxy::~IPAProxy() > > > /** > > > * \brief Retrieve the absolute path to an IPA configuration file > > > * \param[in] name The configuration file name > > > + * \param[in] prefix The configuration directory prefix when searching system paths > > > * > > > * This function locates the configuration file for an IPA and returns its > > > * absolute path. It searches the following directories, in order: > > > @@ -80,8 +81,8 @@ IPAProxy::~IPAProxy() > > > * environment variable ; or > > > * - If libcamera is not installed, the src/ipa/ directory within the source > > > * tree ; otherwise > > > - * - The system sysconf (etc/libcamera/ipa) and the data (share/libcamera/ipa/) > > > - * directories. > > > + * - The system sysconf (etc/libcamera/ipa/<prefix>/) and the data > > > + * (share/libcamera/ipa/<prefix>/) directories. > > > > Doxygen doesn't seem happy with <prefix> > > > > src/libcamera/ipa_proxy.cpp:83: warning: Unsupported xml/html tag <prefix> found > > src/libcamera/ipa_proxy.cpp:84: warning: Unsupported xml/html tag <prefix> found > > Oops! Should I just use "prefix" instead? > > "" > The system sysconf (etc/libcamera/ipa/prefix/) and the data > (share/libcamera/ipa/prefix/) directories. > "" Not sure it <> could be escaped with \< and \> > > Regards, > Naush > > > > > > > > * > > > * The system directories are not searched if libcamera is not installed. > > > * > > > @@ -92,7 +93,8 @@ IPAProxy::~IPAProxy() > > > * \return The full path to the IPA configuration file, or an empty string if > > > * no configuration file can be found > > > */ > > > -std::string IPAProxy::configurationFile(const std::string &name) const > > > +std::string IPAProxy::configurationFile(const std::string &name, > > > + const std::string &prefix) const > > > { > > > struct stat statbuf; > > > int ret; > > > @@ -139,7 +141,8 @@ std::string IPAProxy::configurationFile(const std::string &name) const > > > } else { > > > /* Else look in the system locations. */ > > > for (const auto &dir : utils::split(IPA_CONFIG_DIR, ":")) { > > > - std::string confPath = dir + "/" + ipaName + "/" + name; > > > + std::string confPath = dir + "/" + prefix + "/" + > > > + ipaName + "/" + name; > > > ret = stat(confPath.c_str(), &statbuf); > > > if (ret == 0 && (statbuf.st_mode & S_IFMT) == S_IFREG) > > > return confPath; > > > -- > > > 2.34.1 > > >
Hi Jacopo, On Thu, 27 Apr 2023 at 09:28, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Naush, > what would be the implications of setting the IPA module name to > "rpi/vc4" ? That would make this patch not needed, right ? Are there > undesired implications ? I did try to do this, but sadly it does not work. From https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/ipa_module.cpp#n303 /* Validate the IPA module name. */ std::string ipaName = info_.name; auto iter = std::find_if_not(ipaName.begin(), ipaName.end(), [](unsigned char c) -> bool { return isprint(c) && c != '/' && c != '?' && c != '*' && c != '\\'; }); if (iter != ipaName.end()) { LOG(IPAModule, Error) << "Invalid IPA module name '" << ipaName << "'"; return -EINVAL; } This bit of code complains if the name string has a "/" character. > > On Thu, Apr 27, 2023 at 09:24:16AM +0100, Naushir Patuck wrote: > > On Thu, 27 Apr 2023 at 09:10, Jacopo Mondi > > <jacopo.mondi@ideasonboard.com> wrote: > > > > > > Hi Naush > > > > > > On Thu, Apr 27, 2023 at 08:16:52AM +0100, Naushir Patuck via libcamera-devel wrote: > > > > Add a prefix parameter to IPAProxy::configurationFile(). This prefix is > > > > added to the search path when locating IPA configuration files in the > > > > system directories. > > > > > > > > For example, the system directories etc/libcamera/ipa/<prefix>/ and > > > > share/libcamera/ipa/<prefix>/ will be used to search for the IPA > > > > configuration files. > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > --- > > > > include/libcamera/internal/ipa_proxy.h | 3 ++- > > > > src/libcamera/ipa_proxy.cpp | 11 +++++++---- > > > > 2 files changed, 9 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/include/libcamera/internal/ipa_proxy.h b/include/libcamera/internal/ipa_proxy.h > > > > index 781c8b623605..9235b582edad 100644 > > > > --- a/include/libcamera/internal/ipa_proxy.h > > > > +++ b/include/libcamera/internal/ipa_proxy.h > > > > @@ -31,7 +31,8 @@ public: > > > > > > > > bool isValid() const { return valid_; } > > > > > > > > - std::string configurationFile(const std::string &file) const; > > > > + std::string configurationFile(const std::string &file, > > > > + const std::string &prefix = "") const; > > > > > > > > protected: > > > > std::string resolvePath(const std::string &file) const; > > > > diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp > > > > index 3f2cc6b89f60..4a27b0a993fa 100644 > > > > --- a/src/libcamera/ipa_proxy.cpp > > > > +++ b/src/libcamera/ipa_proxy.cpp > > > > @@ -72,6 +72,7 @@ IPAProxy::~IPAProxy() > > > > /** > > > > * \brief Retrieve the absolute path to an IPA configuration file > > > > * \param[in] name The configuration file name > > > > + * \param[in] prefix The configuration directory prefix when searching system paths > > > > * > > > > * This function locates the configuration file for an IPA and returns its > > > > * absolute path. It searches the following directories, in order: > > > > @@ -80,8 +81,8 @@ IPAProxy::~IPAProxy() > > > > * environment variable ; or > > > > * - If libcamera is not installed, the src/ipa/ directory within the source > > > > * tree ; otherwise > > > > - * - The system sysconf (etc/libcamera/ipa) and the data (share/libcamera/ipa/) > > > > - * directories. > > > > + * - The system sysconf (etc/libcamera/ipa/<prefix>/) and the data > > > > + * (share/libcamera/ipa/<prefix>/) directories. > > > > > > Doxygen doesn't seem happy with <prefix> > > > > > > src/libcamera/ipa_proxy.cpp:83: warning: Unsupported xml/html tag <prefix> found > > > src/libcamera/ipa_proxy.cpp:84: warning: Unsupported xml/html tag <prefix> found > > > > Oops! Should I just use "prefix" instead? > > > > "" > > The system sysconf (etc/libcamera/ipa/prefix/) and the data > > (share/libcamera/ipa/prefix/) directories. > > "" > > Not sure it <> could be escaped with \< and \> Yes, it can! Annoyingly, I forgot that I fixed this very thing for PipelineHandler::configurationFile() some time back! I'll make the update. Given that there are a number of changes now, should I post a v2 with your tags collected? Regards, Naush > > > > Regards, > > Naush > > > > > > > > > > > > * > > > > * The system directories are not searched if libcamera is not installed. > > > > * > > > > @@ -92,7 +93,8 @@ IPAProxy::~IPAProxy() > > > > * \return The full path to the IPA configuration file, or an empty string if > > > > * no configuration file can be found > > > > */ > > > > -std::string IPAProxy::configurationFile(const std::string &name) const > > > > +std::string IPAProxy::configurationFile(const std::string &name, > > > > + const std::string &prefix) const > > > > { > > > > struct stat statbuf; > > > > int ret; > > > > @@ -139,7 +141,8 @@ std::string IPAProxy::configurationFile(const std::string &name) const > > > > } else { > > > > /* Else look in the system locations. */ > > > > for (const auto &dir : utils::split(IPA_CONFIG_DIR, ":")) { > > > > - std::string confPath = dir + "/" + ipaName + "/" + name; > > > > + std::string confPath = dir + "/" + prefix + "/" + > > > > + ipaName + "/" + name; > > > > ret = stat(confPath.c_str(), &statbuf); > > > > if (ret == 0 && (statbuf.st_mode & S_IFMT) == S_IFREG) > > > > return confPath; > > > > -- > > > > 2.34.1 > > > >
On Thu, Apr 27, 2023 at 09:47:02AM +0100, Naushir Patuck via libcamera-devel wrote: > Hi Jacopo, > > On Thu, 27 Apr 2023 at 09:28, Jacopo Mondi > <jacopo.mondi@ideasonboard.com> wrote: > > > > Naush, > > what would be the implications of setting the IPA module name to > > "rpi/vc4" ? That would make this patch not needed, right ? Are there > > undesired implications ? > > I did try to do this, but sadly it does not work. > From https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/ipa_module.cpp#n303 > > /* Validate the IPA module name. */ > std::string ipaName = info_.name; > auto iter = std::find_if_not(ipaName.begin(), ipaName.end(), > [](unsigned char c) -> bool { > return isprint(c) && c != '/' && > c != '?' && c != '*' && > c != '\\'; > }); > if (iter != ipaName.end()) { > LOG(IPAModule, Error) > << "Invalid IPA module name '" << ipaName << "'"; > return -EINVAL; > } > > This bit of code complains if the name string has a "/" character. > AH right. I wonder if we should reconsider that.. For now, unless others are strongly in favour of changing this, let's leave it this way > > > > > On Thu, Apr 27, 2023 at 09:24:16AM +0100, Naushir Patuck wrote: > > > On Thu, 27 Apr 2023 at 09:10, Jacopo Mondi > > > <jacopo.mondi@ideasonboard.com> wrote: > > > > > > > > Hi Naush > > > > > > > > On Thu, Apr 27, 2023 at 08:16:52AM +0100, Naushir Patuck via libcamera-devel wrote: > > > > > Add a prefix parameter to IPAProxy::configurationFile(). This prefix is > > > > > added to the search path when locating IPA configuration files in the > > > > > system directories. > > > > > > > > > > For example, the system directories etc/libcamera/ipa/<prefix>/ and > > > > > share/libcamera/ipa/<prefix>/ will be used to search for the IPA > > > > > configuration files. > > > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > > --- > > > > > include/libcamera/internal/ipa_proxy.h | 3 ++- > > > > > src/libcamera/ipa_proxy.cpp | 11 +++++++---- > > > > > 2 files changed, 9 insertions(+), 5 deletions(-) > > > > > > > > > > diff --git a/include/libcamera/internal/ipa_proxy.h b/include/libcamera/internal/ipa_proxy.h > > > > > index 781c8b623605..9235b582edad 100644 > > > > > --- a/include/libcamera/internal/ipa_proxy.h > > > > > +++ b/include/libcamera/internal/ipa_proxy.h > > > > > @@ -31,7 +31,8 @@ public: > > > > > > > > > > bool isValid() const { return valid_; } > > > > > > > > > > - std::string configurationFile(const std::string &file) const; > > > > > + std::string configurationFile(const std::string &file, > > > > > + const std::string &prefix = "") const; > > > > > > > > > > protected: > > > > > std::string resolvePath(const std::string &file) const; > > > > > diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp > > > > > index 3f2cc6b89f60..4a27b0a993fa 100644 > > > > > --- a/src/libcamera/ipa_proxy.cpp > > > > > +++ b/src/libcamera/ipa_proxy.cpp > > > > > @@ -72,6 +72,7 @@ IPAProxy::~IPAProxy() > > > > > /** > > > > > * \brief Retrieve the absolute path to an IPA configuration file > > > > > * \param[in] name The configuration file name > > > > > + * \param[in] prefix The configuration directory prefix when searching system paths > > > > > * > > > > > * This function locates the configuration file for an IPA and returns its > > > > > * absolute path. It searches the following directories, in order: > > > > > @@ -80,8 +81,8 @@ IPAProxy::~IPAProxy() > > > > > * environment variable ; or > > > > > * - If libcamera is not installed, the src/ipa/ directory within the source > > > > > * tree ; otherwise > > > > > - * - The system sysconf (etc/libcamera/ipa) and the data (share/libcamera/ipa/) > > > > > - * directories. > > > > > + * - The system sysconf (etc/libcamera/ipa/<prefix>/) and the data > > > > > + * (share/libcamera/ipa/<prefix>/) directories. > > > > > > > > Doxygen doesn't seem happy with <prefix> > > > > > > > > src/libcamera/ipa_proxy.cpp:83: warning: Unsupported xml/html tag <prefix> found > > > > src/libcamera/ipa_proxy.cpp:84: warning: Unsupported xml/html tag <prefix> found > > > > > > Oops! Should I just use "prefix" instead? > > > > > > "" > > > The system sysconf (etc/libcamera/ipa/prefix/) and the data > > > (share/libcamera/ipa/prefix/) directories. > > > "" > > > > Not sure it <> could be escaped with \< and \> > > Yes, it can! > > Annoyingly, I forgot that I fixed this very thing for > PipelineHandler::configurationFile() > some time back! > > I'll make the update. Given that there are a number of changes now, should I > post a v2 with your tags collected? > > Regards, > Naush > > > > > > > Regards, > > > Naush > > > > > > > > > > > > > > > > * > > > > > * The system directories are not searched if libcamera is not installed. > > > > > * > > > > > @@ -92,7 +93,8 @@ IPAProxy::~IPAProxy() > > > > > * \return The full path to the IPA configuration file, or an empty string if > > > > > * no configuration file can be found > > > > > */ > > > > > -std::string IPAProxy::configurationFile(const std::string &name) const > > > > > +std::string IPAProxy::configurationFile(const std::string &name, > > > > > + const std::string &prefix) const > > > > > { > > > > > struct stat statbuf; > > > > > int ret; > > > > > @@ -139,7 +141,8 @@ std::string IPAProxy::configurationFile(const std::string &name) const > > > > > } else { > > > > > /* Else look in the system locations. */ > > > > > for (const auto &dir : utils::split(IPA_CONFIG_DIR, ":")) { > > > > > - std::string confPath = dir + "/" + ipaName + "/" + name; > > > > > + std::string confPath = dir + "/" + prefix + "/" + > > > > > + ipaName + "/" + name; > > > > > ret = stat(confPath.c_str(), &statbuf); > > > > > if (ret == 0 && (statbuf.st_mode & S_IFMT) == S_IFREG) > > > > > return confPath; > > > > > -- > > > > > 2.34.1 > > > > >
Hello, Naush, when sending a new version of a patch within a series, could you include the patch sequence count (02/13 in this case) in the subject line, as well a a version number (v1.1 in this case) ? That helps tracking patches. The next version of this patch (unless you post a new version of the whole series, which may be a good idea actually as you've already collected quite a few changes) would be [PATCH v1.2 02/13] libcamera: ipa_proxy: Allow a prefix for the configuration file On Thu, Apr 27, 2023 at 01:03:25PM +0200, Jacopo Mondi via libcamera-devel wrote: > On Thu, Apr 27, 2023 at 09:47:02AM +0100, Naushir Patuck via libcamera-devel wrote: > > On Thu, 27 Apr 2023 at 09:28, Jacopo Mondi wrote: > > > > > > Naush, > > > what would be the implications of setting the IPA module name to > > > "rpi/vc4" ? That would make this patch not needed, right ? Are there > > > undesired implications ? > > > > I did try to do this, but sadly it does not work. > > From https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/ipa_module.cpp#n303 > > > > /* Validate the IPA module name. */ > > std::string ipaName = info_.name; > > auto iter = std::find_if_not(ipaName.begin(), ipaName.end(), > > [](unsigned char c) -> bool { > > return isprint(c) && c != '/' && > > c != '?' && c != '*' && > > c != '\\'; > > }); > > if (iter != ipaName.end()) { > > LOG(IPAModule, Error) > > << "Invalid IPA module name '" << ipaName << "'"; > > return -EINVAL; > > } > > > > This bit of code complains if the name string has a "/" character. > > AH right. I wonder if we should reconsider that.. > For now, unless others are strongly in favour of changing this, let's > leave it this way I do like the idea of handling all this automatically. The constraint on the IPA name comes from commit 6e1cd1394e5d73dfd4c4334cbf8beee79072de21 Author: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Date: Mon Apr 27 04:09:42 2020 +0300 ipa: Name IPA modules after their source directory The IPAModuleInfo::name field is currently a free-formed string that has little use. Tighten its usage rules to make it suitable for building file system paths to IPA-specific resources by matching the directory name of the IPA module. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> The commit expanded the documentation of the IPAModuleInfo::name field with * The name may be used to build file system paths to IPA-specific resources. * It shall only contain printable characters, and may not contain '/', '*', * '?' or '\'. For IPA modules included in libcamera, it shall match the * directory of the IPA module in the source tree. The reason for this is to make it safe(r) to build paths from the IPA module name. I think we could lift the constraint that forbids the '/' character, and instead forbid "..". That should be enough to avoid walking the file system. > > > On Thu, Apr 27, 2023 at 09:24:16AM +0100, Naushir Patuck wrote: > > > > On Thu, 27 Apr 2023 at 09:10, Jacopo Mondi wrote: > > > > > On Thu, Apr 27, 2023 at 08:16:52AM +0100, Naushir Patuck via libcamera-devel wrote: > > > > > > Add a prefix parameter to IPAProxy::configurationFile(). This prefix is > > > > > > added to the search path when locating IPA configuration files in the > > > > > > system directories. > > > > > > > > > > > > For example, the system directories etc/libcamera/ipa/<prefix>/ and > > > > > > share/libcamera/ipa/<prefix>/ will be used to search for the IPA > > > > > > configuration files. > > > > > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > > > --- > > > > > > include/libcamera/internal/ipa_proxy.h | 3 ++- > > > > > > src/libcamera/ipa_proxy.cpp | 11 +++++++---- > > > > > > 2 files changed, 9 insertions(+), 5 deletions(-) > > > > > > > > > > > > diff --git a/include/libcamera/internal/ipa_proxy.h b/include/libcamera/internal/ipa_proxy.h > > > > > > index 781c8b623605..9235b582edad 100644 > > > > > > --- a/include/libcamera/internal/ipa_proxy.h > > > > > > +++ b/include/libcamera/internal/ipa_proxy.h > > > > > > @@ -31,7 +31,8 @@ public: > > > > > > > > > > > > bool isValid() const { return valid_; } > > > > > > > > > > > > - std::string configurationFile(const std::string &file) const; > > > > > > + std::string configurationFile(const std::string &file, > > > > > > + const std::string &prefix = "") const; > > > > > > > > > > > > protected: > > > > > > std::string resolvePath(const std::string &file) const; > > > > > > diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp > > > > > > index 3f2cc6b89f60..4a27b0a993fa 100644 > > > > > > --- a/src/libcamera/ipa_proxy.cpp > > > > > > +++ b/src/libcamera/ipa_proxy.cpp > > > > > > @@ -72,6 +72,7 @@ IPAProxy::~IPAProxy() > > > > > > /** > > > > > > * \brief Retrieve the absolute path to an IPA configuration file > > > > > > * \param[in] name The configuration file name > > > > > > + * \param[in] prefix The configuration directory prefix when searching system paths > > > > > > * > > > > > > * This function locates the configuration file for an IPA and returns its > > > > > > * absolute path. It searches the following directories, in order: > > > > > > @@ -80,8 +81,8 @@ IPAProxy::~IPAProxy() > > > > > > * environment variable ; or > > > > > > * - If libcamera is not installed, the src/ipa/ directory within the source > > > > > > * tree ; otherwise > > > > > > - * - The system sysconf (etc/libcamera/ipa) and the data (share/libcamera/ipa/) > > > > > > - * directories. > > > > > > + * - The system sysconf (etc/libcamera/ipa/<prefix>/) and the data > > > > > > + * (share/libcamera/ipa/<prefix>/) directories. > > > > > > > > > > Doxygen doesn't seem happy with <prefix> > > > > > > > > > > src/libcamera/ipa_proxy.cpp:83: warning: Unsupported xml/html tag <prefix> found > > > > > src/libcamera/ipa_proxy.cpp:84: warning: Unsupported xml/html tag <prefix> found > > > > > > > > Oops! Should I just use "prefix" instead? > > > > > > > > "" > > > > The system sysconf (etc/libcamera/ipa/prefix/) and the data > > > > (share/libcamera/ipa/prefix/) directories. > > > > "" > > > > > > Not sure it <> could be escaped with \< and \> > > > > Yes, it can! > > > > Annoyingly, I forgot that I fixed this very thing for > > PipelineHandler::configurationFile() > > some time back! > > > > I'll make the update. While at it, could you surround the paths with backticks ? * - If libcamera is not installed, the `src/ipa/` directory within the source * tree ; otherwise * - The system sysconf (`etc/libcamera/ipa/\<prefix\>/`) and the data * (`share/libcamera/ipa/\<prefix\>/`) directories. * - The system sysconf (`etc/libcamera/ipa`) and the data * (`share/libcamera/ipa/`) directories. > > Given that there are a number of changes now, should I > > post a v2 with your tags collected? > > > > > > > > * > > > > > > * The system directories are not searched if libcamera is not installed. > > > > > > * > > > > > > @@ -92,7 +93,8 @@ IPAProxy::~IPAProxy() > > > > > > * \return The full path to the IPA configuration file, or an empty string if > > > > > > * no configuration file can be found > > > > > > */ > > > > > > -std::string IPAProxy::configurationFile(const std::string &name) const > > > > > > +std::string IPAProxy::configurationFile(const std::string &name, > > > > > > + const std::string &prefix) const > > > > > > { > > > > > > struct stat statbuf; > > > > > > int ret; > > > > > > @@ -139,7 +141,8 @@ std::string IPAProxy::configurationFile(const std::string &name) const > > > > > > } else { > > > > > > /* Else look in the system locations. */ > > > > > > for (const auto &dir : utils::split(IPA_CONFIG_DIR, ":")) { > > > > > > - std::string confPath = dir + "/" + ipaName + "/" + name; > > > > > > + std::string confPath = dir + "/" + prefix + "/" + > > > > > > + ipaName + "/" + name; > > > > > > ret = stat(confPath.c_str(), &statbuf); > > > > > > if (ret == 0 && (statbuf.st_mode & S_IFMT) == S_IFREG) > > > > > > return confPath;
Hi, On Thu, 27 Apr 2023 at 17:56, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hello, > > Naush, when sending a new version of a patch within a series, could you > include the patch sequence count (02/13 in this case) in the subject > line, as well a a version number (v1.1 in this case) ? That helps > tracking patches. The next version of this patch (unless you post a new > version of the whole series, which may be a good idea actually as you've > already collected quite a few changes) would be > > [PATCH v1.2 02/13] libcamera: ipa_proxy: Allow a prefix for the configuration file > > On Thu, Apr 27, 2023 at 01:03:25PM +0200, Jacopo Mondi via libcamera-devel wrote: > > On Thu, Apr 27, 2023 at 09:47:02AM +0100, Naushir Patuck via libcamera-devel wrote: > > > On Thu, 27 Apr 2023 at 09:28, Jacopo Mondi wrote: > > > > > > > > Naush, > > > > what would be the implications of setting the IPA module name to > > > > "rpi/vc4" ? That would make this patch not needed, right ? Are there > > > > undesired implications ? > > > > > > I did try to do this, but sadly it does not work. > > > From https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/ipa_module.cpp#n303 > > > > > > /* Validate the IPA module name. */ > > > std::string ipaName = info_.name; > > > auto iter = std::find_if_not(ipaName.begin(), ipaName.end(), > > > [](unsigned char c) -> bool { > > > return isprint(c) && c != '/' && > > > c != '?' && c != '*' && > > > c != '\\'; > > > }); > > > if (iter != ipaName.end()) { > > > LOG(IPAModule, Error) > > > << "Invalid IPA module name '" << ipaName << "'"; > > > return -EINVAL; > > > } > > > > > > This bit of code complains if the name string has a "/" character. > > > > AH right. I wonder if we should reconsider that.. > > For now, unless others are strongly in favour of changing this, let's > > leave it this way > > I do like the idea of handling all this automatically. > > The constraint on the IPA name comes from > > commit 6e1cd1394e5d73dfd4c4334cbf8beee79072de21 > Author: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Date: Mon Apr 27 04:09:42 2020 +0300 > > ipa: Name IPA modules after their source directory > > The IPAModuleInfo::name field is currently a free-formed string that has > little use. Tighten its usage rules to make it suitable for building > file system paths to IPA-specific resources by matching the directory > name of the IPA module. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > The commit expanded the documentation of the IPAModuleInfo::name field > with > > * The name may be used to build file system paths to IPA-specific resources. > * It shall only contain printable characters, and may not contain '/', '*', > * '?' or '\'. For IPA modules included in libcamera, it shall match the > * directory of the IPA module in the source tree. > > The reason for this is to make it safe(r) to build paths from the IPA > module name. > > I think we could lift the constraint that forbids the '/' character, and > instead forbid "..". That should be enough to avoid walking the file > system. > Ok, I'll add a change to remove the "/" constraint and use "rpi/vc4" as the IPA name. We can then remove this commit from the series. Naush > > > > On Thu, Apr 27, 2023 at 09:24:16AM +0100, Naushir Patuck wrote: > > > > > On Thu, 27 Apr 2023 at 09:10, Jacopo Mondi wrote: > > > > > > On Thu, Apr 27, 2023 at 08:16:52AM +0100, Naushir Patuck via libcamera-devel wrote: > > > > > > > Add a prefix parameter to IPAProxy::configurationFile(). This prefix is > > > > > > > added to the search path when locating IPA configuration files in the > > > > > > > system directories. > > > > > > > > > > > > > > For example, the system directories etc/libcamera/ipa/<prefix>/ and > > > > > > > share/libcamera/ipa/<prefix>/ will be used to search for the IPA > > > > > > > configuration files. > > > > > > > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > > > > --- > > > > > > > include/libcamera/internal/ipa_proxy.h | 3 ++- > > > > > > > src/libcamera/ipa_proxy.cpp | 11 +++++++---- > > > > > > > 2 files changed, 9 insertions(+), 5 deletions(-) > > > > > > > > > > > > > > diff --git a/include/libcamera/internal/ipa_proxy.h b/include/libcamera/internal/ipa_proxy.h > > > > > > > index 781c8b623605..9235b582edad 100644 > > > > > > > --- a/include/libcamera/internal/ipa_proxy.h > > > > > > > +++ b/include/libcamera/internal/ipa_proxy.h > > > > > > > @@ -31,7 +31,8 @@ public: > > > > > > > > > > > > > > bool isValid() const { return valid_; } > > > > > > > > > > > > > > - std::string configurationFile(const std::string &file) const; > > > > > > > + std::string configurationFile(const std::string &file, > > > > > > > + const std::string &prefix = "") const; > > > > > > > > > > > > > > protected: > > > > > > > std::string resolvePath(const std::string &file) const; > > > > > > > diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp > > > > > > > index 3f2cc6b89f60..4a27b0a993fa 100644 > > > > > > > --- a/src/libcamera/ipa_proxy.cpp > > > > > > > +++ b/src/libcamera/ipa_proxy.cpp > > > > > > > @@ -72,6 +72,7 @@ IPAProxy::~IPAProxy() > > > > > > > /** > > > > > > > * \brief Retrieve the absolute path to an IPA configuration file > > > > > > > * \param[in] name The configuration file name > > > > > > > + * \param[in] prefix The configuration directory prefix when searching system paths > > > > > > > * > > > > > > > * This function locates the configuration file for an IPA and returns its > > > > > > > * absolute path. It searches the following directories, in order: > > > > > > > @@ -80,8 +81,8 @@ IPAProxy::~IPAProxy() > > > > > > > * environment variable ; or > > > > > > > * - If libcamera is not installed, the src/ipa/ directory within the source > > > > > > > * tree ; otherwise > > > > > > > - * - The system sysconf (etc/libcamera/ipa) and the data (share/libcamera/ipa/) > > > > > > > - * directories. > > > > > > > + * - The system sysconf (etc/libcamera/ipa/<prefix>/) and the data > > > > > > > + * (share/libcamera/ipa/<prefix>/) directories. > > > > > > > > > > > > Doxygen doesn't seem happy with <prefix> > > > > > > > > > > > > src/libcamera/ipa_proxy.cpp:83: warning: Unsupported xml/html tag <prefix> found > > > > > > src/libcamera/ipa_proxy.cpp:84: warning: Unsupported xml/html tag <prefix> found > > > > > > > > > > Oops! Should I just use "prefix" instead? > > > > > > > > > > "" > > > > > The system sysconf (etc/libcamera/ipa/prefix/) and the data > > > > > (share/libcamera/ipa/prefix/) directories. > > > > > "" > > > > > > > > Not sure it <> could be escaped with \< and \> > > > > > > Yes, it can! > > > > > > Annoyingly, I forgot that I fixed this very thing for > > > PipelineHandler::configurationFile() > > > some time back! > > > > > > I'll make the update. > > While at it, could you surround the paths with backticks ? > > * - If libcamera is not installed, the `src/ipa/` directory within the source > * tree ; otherwise > * - The system sysconf (`etc/libcamera/ipa/\<prefix\>/`) and the data > * (`share/libcamera/ipa/\<prefix\>/`) directories. > * - The system sysconf (`etc/libcamera/ipa`) and the data > * (`share/libcamera/ipa/`) directories. > > > > Given that there are a number of changes now, should I > > > post a v2 with your tags collected? > > > > > > > > > > * > > > > > > > * The system directories are not searched if libcamera is not installed. > > > > > > > * > > > > > > > @@ -92,7 +93,8 @@ IPAProxy::~IPAProxy() > > > > > > > * \return The full path to the IPA configuration file, or an empty string if > > > > > > > * no configuration file can be found > > > > > > > */ > > > > > > > -std::string IPAProxy::configurationFile(const std::string &name) const > > > > > > > +std::string IPAProxy::configurationFile(const std::string &name, > > > > > > > + const std::string &prefix) const > > > > > > > { > > > > > > > struct stat statbuf; > > > > > > > int ret; > > > > > > > @@ -139,7 +141,8 @@ std::string IPAProxy::configurationFile(const std::string &name) const > > > > > > > } else { > > > > > > > /* Else look in the system locations. */ > > > > > > > for (const auto &dir : utils::split(IPA_CONFIG_DIR, ":")) { > > > > > > > - std::string confPath = dir + "/" + ipaName + "/" + name; > > > > > > > + std::string confPath = dir + "/" + prefix + "/" + > > > > > > > + ipaName + "/" + name; > > > > > > > ret = stat(confPath.c_str(), &statbuf); > > > > > > > if (ret == 0 && (statbuf.st_mode & S_IFMT) == S_IFREG) > > > > > > > return confPath; > > -- > Regards, > > Laurent Pinchart
diff --git a/include/libcamera/internal/ipa_proxy.h b/include/libcamera/internal/ipa_proxy.h index 781c8b623605..9235b582edad 100644 --- a/include/libcamera/internal/ipa_proxy.h +++ b/include/libcamera/internal/ipa_proxy.h @@ -31,7 +31,8 @@ public: bool isValid() const { return valid_; } - std::string configurationFile(const std::string &file) const; + std::string configurationFile(const std::string &file, + const std::string &prefix = "") const; protected: std::string resolvePath(const std::string &file) const; diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp index 3f2cc6b89f60..4a27b0a993fa 100644 --- a/src/libcamera/ipa_proxy.cpp +++ b/src/libcamera/ipa_proxy.cpp @@ -72,6 +72,7 @@ IPAProxy::~IPAProxy() /** * \brief Retrieve the absolute path to an IPA configuration file * \param[in] name The configuration file name + * \param[in] prefix The configuration directory prefix when searching system paths * * This function locates the configuration file for an IPA and returns its * absolute path. It searches the following directories, in order: @@ -80,8 +81,8 @@ IPAProxy::~IPAProxy() * environment variable ; or * - If libcamera is not installed, the src/ipa/ directory within the source * tree ; otherwise - * - The system sysconf (etc/libcamera/ipa) and the data (share/libcamera/ipa/) - * directories. + * - The system sysconf (etc/libcamera/ipa/<prefix>/) and the data + * (share/libcamera/ipa/<prefix>/) directories. * * The system directories are not searched if libcamera is not installed. * @@ -92,7 +93,8 @@ IPAProxy::~IPAProxy() * \return The full path to the IPA configuration file, or an empty string if * no configuration file can be found */ -std::string IPAProxy::configurationFile(const std::string &name) const +std::string IPAProxy::configurationFile(const std::string &name, + const std::string &prefix) const { struct stat statbuf; int ret; @@ -139,7 +141,8 @@ std::string IPAProxy::configurationFile(const std::string &name) const } else { /* Else look in the system locations. */ for (const auto &dir : utils::split(IPA_CONFIG_DIR, ":")) { - std::string confPath = dir + "/" + ipaName + "/" + name; + std::string confPath = dir + "/" + prefix + "/" + + ipaName + "/" + name; ret = stat(confPath.c_str(), &statbuf); if (ret == 0 && (statbuf.st_mode & S_IFMT) == S_IFREG) return confPath;
Add a prefix parameter to IPAProxy::configurationFile(). This prefix is added to the search path when locating IPA configuration files in the system directories. For example, the system directories etc/libcamera/ipa/<prefix>/ and share/libcamera/ipa/<prefix>/ will be used to search for the IPA configuration files. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- include/libcamera/internal/ipa_proxy.h | 3 ++- src/libcamera/ipa_proxy.cpp | 11 +++++++---- 2 files changed, 9 insertions(+), 5 deletions(-)