[{"id":2138,"web_url":"https://patchwork.libcamera.org/comment/2138/","msgid":"<20190703222714.GL5007@pendragon.ideasonboard.com>","date":"2019-07-03T22:27:14","subject":"Re: [libcamera-devel] [RFC PATCH v2 5/7] libcamera: ipa_manager:\n\tuse proxy","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Wed, Jul 03, 2019 at 05:00:05PM +0900, Paul Elder wrote:\n> Make IPAManager isolate an IPA in a Proxy if the IPA's license is not\n> open source, before returning the IPA to the caller. For now, only use\n> the default Linux proxy, and only GPL and LGPL are considered open\n> source.\n> \n> Signed-off-by: Paul Elder \n> ---\n> New in v2\n> - replaces adding shims\n> - since Proxies are not external shared objects like the shims in v1\n> were, there is no longer a list of shims that is treated like\n> IPAModules\n> - instead the matching is done by searching the list of proxy factories\n> \n> src/libcamera/ipa_manager.cpp | 48 +++++++++++++++++++++++++++++++++--\n> 1 file changed, 46 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp\n> index 532b77d..60cd84d 100644\n> --- a/src/libcamera/ipa_manager.cpp\n> +++ b/src/libcamera/ipa_manager.cpp\n> @@ -14,6 +14,7 @@\n> #include \"ipa_module.h\"\n> #include \"log.h\"\n> #include \"pipeline_handler.h\"\n> +#include \"ipa_proxy.h\"\n\nAlphabetical order please.\n\n> #include \"utils.h\"\n> \n> /**\n> @@ -25,6 +26,20 @@ namespace libcamera {\n> \n> LOG_DEFINE_CATEGORY(IPAManager)\n> \n> +namespace {\n> +\n> +bool isOpenSource(const char *license)\n> +{\n> +\tif (!strncmp(license, \"GPL\", 3))\n> +\t\treturn true;\n> +\tif (!strncmp(license, \"LGPL\", 4))\n> +\t\treturn true;\n> +\n> +\treturn false;\n> +}\n\nYou could add this method to the IPAModule class. It would then have to\nbe documented, which would ensure the documentation is explicit about\nwhat licenses are considered open-source.\n\nAs you only accept GPL licenses for now, should the method be named\nisGPL() ?\n\n> +\n> +} /* namespace */\n> +\n> /**\n> * \\class IPAManager\n> * \\brief Manager for IPA modules\n> @@ -129,7 +144,7 @@ int IPAManager::addDir(const char *libDir)\n> * \\param[in] maxVersion Maximum acceptable version of IPA module\n> *\n> * \\return A newly created IPA interface, or nullptr if no matching\n> - * IPA module is found\n> + * IPA module is found or if the IPA interface fails to initialize\n\n\"initialize\" seems to imply the init() method, which isn't called here.\nHow about \"if no matching IPA module is found or if the module interface\ncan't be accessed\" ? I'm still not very happy with that sentence, so if\nyou have a better idea please propose it.\n\n> */\n> std::unique_ptr IPAManager::createIPA(PipelineHandler *pipe,\n> \t\t\t\t\t\t uint32_t maxVersion,\n> @@ -144,7 +159,36 @@ std::unique_ptr IPAManager::createIPA(PipelineHandler *pipe,\n> \t\t}\n> \t}\n> \n> -\tif (!m || !m->load())\n> +\tif (!m)\n> +\t\treturn nullptr;\n> +\n> +\tif (!isOpenSource(m->info().license)) {\n> +\t\tProxyFactory *pf = nullptr;\n> +\t\tstd::vector &factories = ProxyFactory::factories();\n> +\n> +\t\tfor (ProxyFactory *factory : factories) {\n\nYou can write\n\n\t\tfor (ProxyFactor *factory : ProxyFactory::factories()) {\n\n> +\t\t\t/* TODO: Better matching */\n\n\t\t\t/**\n\t\t\t * \\todo Match proxies with modules through a\n\t\t\t * configuration file.\n\t\t\t */\n\n> +\t\t\tif (!strcmp(factory->name().c_str(), \"ProxyLinuxDefault\")) {\n\n\t\t\tif (factory->name() == \"ProxyLinuxDefault\") {\n\n> +\t\t\t\tpf = factory;\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\t}\n> +\n> +\t\tif (!pf) {\n> +\t\t\tLOG(IPAManager, Error) << \"Failed to get proxy factory\";\n\n\"Failed to find IPA proxy factory for module \" << m->libPath();\n\nWhich requires exposing a libPath() method to the IPAModule class, but I\nthink you will need that anyway as the configuration file will likely\nassociate a library path with a proxy (we can't trust the name reported\nby the module itself, or any information it contains for that matter, to\ndecide which proxy to use).)\n\n> +\t\t\treturn nullptr;\n> +\t\t}\n> +\n> +\t\tstd::unique_ptr proxy = pf->create(m);\n> +\t\tif (!proxy->isValid()) {\n> +\t\t\tLOG(IPAManager, Error) << \"Failed to load proxy\";\n\n\"Failed to create IPA proxy for module \" << m->libPath();\n\n> +\t\t\treturn nullptr;\n> +\t\t}\n> +\n> +\t\treturn proxy;\n> +\t}\n> +\n> +\tif (!m->load())\n> \t\treturn nullptr;\n> \n> \treturn m->createInstance();","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 32F8460C01\n\tfor ;\n\tThu, 4 Jul 2019 00:27:35 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A038224B;\n\tThu, 4 Jul 2019 00:27:34 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1562192854;\n\tbh=dsA+oTEiOttxkonoxqz1wvAKOxB0u/fBCQiI0eeDb8o=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=tK+PdpdRFvzOMxTEWLx9N7qR2XvT5QIogSvSVmC+T0eXXaue8dKCrWNk6/h9KbYx9\n\tQc/UncZUyUtcWYrmZDedPzQs84bDn2BkpGhFPMXHzo8ZyrD8MM6vRkQCTJuNFyqaUW\n\tIsoOYlPpnA9pM30NeHHaNn9hDwyynxSG10FMas7s=","Date":"Thu, 4 Jul 2019 01:27:14 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190703222714.GL5007@pendragon.ideasonboard.com>","References":"<20190703080007.21376-1-paul.elder@ideasonboard.com>\n\t<20190703080007.21376-6-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190703080007.21376-6-paul.elder@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [RFC PATCH v2 5/7] libcamera: ipa_manager:\n\tuse proxy","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 03 Jul 2019 22:27:35 -0000"}}]