[libcamera-devel] [PATCH] libcamera: ipa_proxy: Allow a prefix for the configuration file
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Apr 27 18:56:57 CEST 2023
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 at 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 at ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham at 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 at 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
More information about the libcamera-devel
mailing list