[libcamera-devel,3/3] libcamera: ipa_manager: Allow forcing IPA module isolation
diff mbox series

Message ID 20210711231547.19664-4-laurent.pinchart@ideasonboard.com
State Accepted
Headers show
Series
  • libcamera: ipa_manager: Fix, cleanup, and allow forcing isolation
Related show

Commit Message

Laurent Pinchart July 11, 2021, 11:15 p.m. UTC 
For test purpose it's useful to run open-source IPA modules in
isolation. This can already be done by deleting the corresponding
signature file, but that method can be inconvenient. Add a way to force
IPA module isolation through a new LIBCAMERA_IPA_FORCE_ISOLATION
environment variable.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 Documentation/environment_variables.rst | 5 +++++
 src/libcamera/ipa_manager.cpp           | 8 ++++++++
 2 files changed, 13 insertions(+)

Comments

Kieran Bingham July 12, 2021, 7:55 a.m. UTC | #1 
Hi Laurent,

On 12/07/2021 00:15, Laurent Pinchart wrote:
> For test purpose it's useful to run open-source IPA modules in
> isolation. This can already be done by deleting the corresponding
> signature file, but that method can be inconvenient. Add a way to force
> IPA module isolation through a new LIBCAMERA_IPA_FORCE_ISOLATION
> environment variable.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  Documentation/environment_variables.rst | 5 +++++
>  src/libcamera/ipa_manager.cpp           | 8 ++++++++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/Documentation/environment_variables.rst b/Documentation/environment_variables.rst
> index d392fd26b87a..1e85befd538a 100644
> --- a/Documentation/environment_variables.rst
> +++ b/Documentation/environment_variables.rst
> @@ -24,6 +24,11 @@ LIBCAMERA_IPA_CONFIG_PATH
>  
>     Example value: ``${HOME}/.libcamera/share/ipa:/opt/libcamera/vendor/share/ipa``
>  
> +LIBCAMERA_IPA_FORCE_ISOLATION
> +   When set to a non-empty string, force process isolation of all IPA modules.
> +
> +   Example value: ``1``
> +
>  LIBCAMERA_IPA_MODULE_PATH
>     Define custom search locations for IPA modules (`more <IPA module_>`__).
>  
> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
> index 9533c8fadea6..028b2ce21779 100644
> --- a/src/libcamera/ipa_manager.cpp
> +++ b/src/libcamera/ipa_manager.cpp
> @@ -276,6 +276,14 @@ IPAModule *IPAManager::module(PipelineHandler *pipe, uint32_t minVersion,
>  bool IPAManager::isSignatureValid([[maybe_unused]] IPAModule *ipa) const
>  {
>  #if HAVE_IPA_PUBKEY
> +	char *force = utils::secure_getenv("LIBCAMERA_IPA_FORCE_ISOLATION");
> +	if (force && force[0] != '\0') {

We're getting this pattern a lot. I wonder if we should have
utils::secure_getenv return null if env[0] == '\0' to simplify the code.

Or otherwise, have an isSet() wrapper.

But anyway, that's not required here.

I suspect that the users of this env won't extend beyond developers and
tests, but I think even just for testing that provides a very valid use
case here, as we'll be able to create comparable tests using the same
IPA but for both isolated, and non-isolated cases.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>


> +		LOG(IPAManager, Debug)
> +			<< "Isolation of IPA module " << ipa->path()
> +			<< " forced through environment variable";
> +		return false;
> +	}
> +
>  	File file{ ipa->path() };
>  	if (!file.open(File::ReadOnly))
>  		return false;
>
Laurent Pinchart July 12, 2021, 10:02 p.m. UTC | #2 
Hi Kieran,

On Mon, Jul 12, 2021 at 08:55:01AM +0100, Kieran Bingham wrote:
> On 12/07/2021 00:15, Laurent Pinchart wrote:
> > For test purpose it's useful to run open-source IPA modules in
> > isolation. This can already be done by deleting the corresponding
> > signature file, but that method can be inconvenient. Add a way to force
> > IPA module isolation through a new LIBCAMERA_IPA_FORCE_ISOLATION
> > environment variable.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  Documentation/environment_variables.rst | 5 +++++
> >  src/libcamera/ipa_manager.cpp           | 8 ++++++++
> >  2 files changed, 13 insertions(+)
> > 
> > diff --git a/Documentation/environment_variables.rst b/Documentation/environment_variables.rst
> > index d392fd26b87a..1e85befd538a 100644
> > --- a/Documentation/environment_variables.rst
> > +++ b/Documentation/environment_variables.rst
> > @@ -24,6 +24,11 @@ LIBCAMERA_IPA_CONFIG_PATH
> >  
> >     Example value: ``${HOME}/.libcamera/share/ipa:/opt/libcamera/vendor/share/ipa``
> >  
> > +LIBCAMERA_IPA_FORCE_ISOLATION
> > +   When set to a non-empty string, force process isolation of all IPA modules.
> > +
> > +   Example value: ``1``
> > +
> >  LIBCAMERA_IPA_MODULE_PATH
> >     Define custom search locations for IPA modules (`more <IPA module_>`__).
> >  
> > diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
> > index 9533c8fadea6..028b2ce21779 100644
> > --- a/src/libcamera/ipa_manager.cpp
> > +++ b/src/libcamera/ipa_manager.cpp
> > @@ -276,6 +276,14 @@ IPAModule *IPAManager::module(PipelineHandler *pipe, uint32_t minVersion,
> >  bool IPAManager::isSignatureValid([[maybe_unused]] IPAModule *ipa) const
> >  {
> >  #if HAVE_IPA_PUBKEY
> > +	char *force = utils::secure_getenv("LIBCAMERA_IPA_FORCE_ISOLATION");
> > +	if (force && force[0] != '\0') {
> 
> We're getting this pattern a lot. I wonder if we should have
> utils::secure_getenv return null if env[0] == '\0' to simplify the code.
> 
> Or otherwise, have an isSet() wrapper.

I wouldn't integrate it in utils::secure_getenv() as it would depart
from the secure_getenv() (and getenv()) API, possibly leading to subtle
bugs. A wrapper would be a better idea I think. I'm tempted to defer
addressing this to when we'll need more helper functions to manage the
environment variables.

> But anyway, that's not required here.
> 
> I suspect that the users of this env won't extend beyond developers and
> tests, but I think even just for testing that provides a very valid use
> case here, as we'll be able to create comparable tests using the same
> IPA but for both isolated, and non-isolated cases.

Yes, this is really for testing. I got the idea from a DNI patch from
Umang that hardcoded a return false; here for testing.

> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> > +		LOG(IPAManager, Debug)
> > +			<< "Isolation of IPA module " << ipa->path()
> > +			<< " forced through environment variable";
> > +		return false;
> > +	}
> > +
> >  	File file{ ipa->path() };
> >  	if (!file.open(File::ReadOnly))
> >  		return false;
Paul Elder July 19, 2021, 3:21 a.m. UTC | #3 
Hi Laurent,

On Mon, Jul 12, 2021 at 02:15:47AM +0300, Laurent Pinchart wrote:
> For test purpose it's useful to run open-source IPA modules in
> isolation. This can already be done by deleting the corresponding
> signature file, but that method can be inconvenient. Add a way to force
> IPA module isolation through a new LIBCAMERA_IPA_FORCE_ISOLATION
> environment variable.

Ooh, that's convenient.

> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> ---
>  Documentation/environment_variables.rst | 5 +++++
>  src/libcamera/ipa_manager.cpp           | 8 ++++++++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/Documentation/environment_variables.rst b/Documentation/environment_variables.rst
> index d392fd26b87a..1e85befd538a 100644
> --- a/Documentation/environment_variables.rst
> +++ b/Documentation/environment_variables.rst
> @@ -24,6 +24,11 @@ LIBCAMERA_IPA_CONFIG_PATH
>  
>     Example value: ``${HOME}/.libcamera/share/ipa:/opt/libcamera/vendor/share/ipa``
>  
> +LIBCAMERA_IPA_FORCE_ISOLATION
> +   When set to a non-empty string, force process isolation of all IPA modules.
> +
> +   Example value: ``1``
> +
>  LIBCAMERA_IPA_MODULE_PATH
>     Define custom search locations for IPA modules (`more <IPA module_>`__).
>  
> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
> index 9533c8fadea6..028b2ce21779 100644
> --- a/src/libcamera/ipa_manager.cpp
> +++ b/src/libcamera/ipa_manager.cpp
> @@ -276,6 +276,14 @@ IPAModule *IPAManager::module(PipelineHandler *pipe, uint32_t minVersion,
>  bool IPAManager::isSignatureValid([[maybe_unused]] IPAModule *ipa) const
>  {
>  #if HAVE_IPA_PUBKEY
> +	char *force = utils::secure_getenv("LIBCAMERA_IPA_FORCE_ISOLATION");
> +	if (force && force[0] != '\0') {
> +		LOG(IPAManager, Debug)
> +			<< "Isolation of IPA module " << ipa->path()
> +			<< " forced through environment variable";
> +		return false;
> +	}
> +
>  	File file{ ipa->path() };
>  	if (!file.open(File::ReadOnly))
>  		return false;
> -- 
> Regards,
> 
> Laurent Pinchart
>

Patch
diff mbox series 

diff --git a/Documentation/environment_variables.rst b/Documentation/environment_variables.rst
index d392fd26b87a..1e85befd538a 100644
--- a/Documentation/environment_variables.rst
+++ b/Documentation/environment_variables.rst
@@ -24,6 +24,11 @@  LIBCAMERA_IPA_CONFIG_PATH
 
    Example value: ``${HOME}/.libcamera/share/ipa:/opt/libcamera/vendor/share/ipa``
 
+LIBCAMERA_IPA_FORCE_ISOLATION
+   When set to a non-empty string, force process isolation of all IPA modules.
+
+   Example value: ``1``
+
 LIBCAMERA_IPA_MODULE_PATH
    Define custom search locations for IPA modules (`more <IPA module_>`__).
 
diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
index 9533c8fadea6..028b2ce21779 100644
--- a/src/libcamera/ipa_manager.cpp
+++ b/src/libcamera/ipa_manager.cpp
@@ -276,6 +276,14 @@  IPAModule *IPAManager::module(PipelineHandler *pipe, uint32_t minVersion,
 bool IPAManager::isSignatureValid([[maybe_unused]] IPAModule *ipa) const
 {
 #if HAVE_IPA_PUBKEY
+	char *force = utils::secure_getenv("LIBCAMERA_IPA_FORCE_ISOLATION");
+	if (force && force[0] != '\0') {
+		LOG(IPAManager, Debug)
+			<< "Isolation of IPA module " << ipa->path()
+			<< " forced through environment variable";
+		return false;
+	}
+
 	File file{ ipa->path() };
 	if (!file.open(File::ReadOnly))
 		return false;