[libcamera-devel] [PATCH 3/3] libcamera: ipa_manager: Allow forcing IPA module isolation

[Laurent Pinchart]

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 at 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 at 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;

-- 
Regards,

Laurent Pinchart