[libcamera-devel] [PATCH 01/11] ipa: Name IPA modules after their source directory

[Jacopo Mondi]

Hi Laurent,

On Mon, Apr 27, 2020 at 06:17:03AM +0300, Laurent Pinchart wrote:
> 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.
>

I wonder, should we use 'name' for this purpose, or should we get the
file name directly, strip the .cpp extension out and add a field for
that purpose ? I feel maintaining the name in synch with the file name
is a (relatively small) burden we could avoid.

> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  src/ipa/rkisp1/rkisp1.cpp    |  2 +-
>  src/ipa/vimc/vimc.cpp        |  2 +-
>  src/libcamera/ipa_module.cpp | 20 ++++++++++++++++++++
>  test/ipa/ipa_module_test.cpp |  2 +-
>  4 files changed, 23 insertions(+), 3 deletions(-)
>
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index acbbe58c7a2e..7f0ffb0a791f 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -274,7 +274,7 @@ const struct IPAModuleInfo ipaModuleInfo = {
>  	IPA_MODULE_API_VERSION,
>  	1,
>  	"PipelineHandlerRkISP1",
> -	"RkISP1 IPA",
> +	"rkisp1",
>  };
>
>  struct ipa_context *ipaCreate()
> diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp
> index d2267e11737f..cfdbd6f99155 100644
> --- a/src/ipa/vimc/vimc.cpp
> +++ b/src/ipa/vimc/vimc.cpp
> @@ -126,7 +126,7 @@ const struct IPAModuleInfo ipaModuleInfo = {
>  	IPA_MODULE_API_VERSION,
>  	0,
>  	"PipelineHandlerVimc",
> -	"Dummy IPA for Vimc",
> +	"vimc",
>  };
>
>  struct ipa_context *ipaCreate()
> diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp
> index 958ede74688e..52823e88a508 100644
> --- a/src/libcamera/ipa_module.cpp
> +++ b/src/libcamera/ipa_module.cpp
> @@ -9,6 +9,7 @@
>
>  #include <algorithm>
>  #include <array>
> +#include <ctype.h>
>  #include <dlfcn.h>
>  #include <elf.h>
>  #include <errno.h>
> @@ -216,6 +217,11 @@ Span<uint8_t> elfLoadSymbol(Span<uint8_t> elf, const char *symbol)
>   * \var IPAModuleInfo::name
>   * \brief The name of the IPA module
>   *
> + * 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.
> + *
>   * \todo Allow user to choose to isolate open source IPAs
>   */
>
> @@ -287,6 +293,20 @@ int IPAModule::loadIPAModuleInfo()
>  		return -EINVAL;
>  	}
>
> +	/* 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;
> +	}
> +
>  	/* Load the signature. Failures are not fatal. */
>  	File sign{ libPath_ + ".sign" };
>  	if (!sign.open(File::ReadOnly)) {
> diff --git a/test/ipa/ipa_module_test.cpp b/test/ipa/ipa_module_test.cpp
> index d22c302fa726..e3aee190b410 100644
> --- a/test/ipa/ipa_module_test.cpp
> +++ b/test/ipa/ipa_module_test.cpp
> @@ -58,7 +58,7 @@ protected:
>  			IPA_MODULE_API_VERSION,
>  			0,
>  			"PipelineHandlerVimc",
> -			"Dummy IPA for Vimc",
> +			"vimc",
>  		};
>
>  		count += runTest("src/ipa/vimc/ipa_vimc.so", testInfo);
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel