[libcamera-devel] [PATCH] libcamera: ipa_proxy: Allow a prefix for the configuration file
Naushir Patuck
naush at raspberrypi.com
Fri Apr 28 11:11:13 CEST 2023
Hi,
On Thu, 27 Apr 2023 at 17:56, Laurent Pinchart
<laurent.pinchart at 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 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.
>
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 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