[{"id":1782,"web_url":"https://patchwork.libcamera.org/comment/1782/","msgid":"<20190606101606.GG12825@pendragon.ideasonboard.com>","date":"2019-06-06T10:16:06","subject":"Re: [libcamera-devel] [RFC PATCH 10/10] libcamera: ipa: shim: load\n\tIPA module into an IPAInterface","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, Jun 05, 2019 at 06:18:17PM -0400, Paul Elder wrote:\n> Implement the dummy shim's init method that takes a path for the IPA\n> module to be loaded. Set up IPC, then fork and load the IPAInterface\n> implementation from the IPA module shared object.\n> \n> Implement a cleanup method along with it.\n> \n> Signed-off-by: Paul Elder \n\nI understand you've kept this patch separate as it's a bit of an early\ndraft, but I think it should be squashed with the patch that adds the\ndummy shim, as it makes little sense to have a dummy implementation that\ndoes nothing. I would also rename it to something else than dummy, may\n-linux-default or something similar ?\n\n\n> ---\n> This is the most draft-y patch of the whole series. I wasn't sure how to\n> put what kind of IPC, so I kind of just did this very skeletal \"give you\n> an idea\" type of IPC initialization. Privilege dropping will be looked\n> into for the next version.\n> \n> src/ipa/shim_dummy.cpp | 79 ++++++++++++++++++++++++++++++++++++++++--\n> 1 file changed, 77 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/ipa/shim_dummy.cpp b/src/ipa/shim_dummy.cpp\n> index 4d28c2d..f2e6961 100644\n> --- a/src/ipa/shim_dummy.cpp\n> +++ b/src/ipa/shim_dummy.cpp\n> @@ -6,6 +6,14 @@\n> */\n> \n> #include \n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> \n> #include \n> #include \n> @@ -17,20 +25,87 @@ class ShimDummy : public IPAInterface\n> public:\n> \tint init();\n> \tint init(const char *path);\n> +\n> +\tvoid cleanup();\n> +\n> +private:\n> +\tstd::unique_ptr ipa_;\n> +\n> +\tint sockets_[2];\n> +\tint childPid_;\n> +\tvoid *dlHandle_;\n> +\n> +\ttypedef IPAInterface *(*IPAIntfFactory)(void);\n> };\n> \n> int ShimDummy::init()\n> {\n> -\tstd::cout << \"okay shim init without path\" << std::endl;\n> +\tstd::cout << \"initializing IPA via dummy shim!\" << std::endl;\n> \treturn 0;\n> }\n> \n> int ShimDummy::init(const char *path)\n> {\n> -\tstd::cout << \"initializing dummy shim!\" << std::endl;\n> +\tstd::cout << \"initializing dummy shim! loading IPA from \" << path\n> +\t\t << std::endl;\n> +\n> +\t/* We know the IPA module is valid, otherwise IPAManager wouldn't\n> +\t * even give its path to us. */\n> +\n> +\t// setup comm\n> +\tif (socketpair(AF_UNIX, SOCK_STREAM, 0, sockets_)) {\n> +\t\tint err = errno;\n> +\t\tstd::cerr << \"Error opening sockets: \" << strerror(errno)\n> +\t\t\t << std::endl;\n> +\t\treturn err;\n> +\t}\n> +\n> +\t// fork\n\nI would separate IPC and process management in different functions, as\nboth will grow. I even think you should have separate classes to handle\nthose.\n\nOne item that needs to be researched is how to handle all the file\ndescriptors created by the application that should not passed to the\nchild process.\n\n> +\tif ((childPid_ = fork()) == -1) {\n> +\t\tint err = errno;\n> +\t\tstd::cerr << \"Failed to fork: \" << strerror(errno) << std::endl;\n> +\t\treturn err;\n> +\t} else if (childPid_) {\n> +\t\t// we are parent\n> +\t\t// we can read/write sockets_[1]\n> +\t\tclose(sockets_[0]);\n> +\t} else {\n> +\t\t// we are child\n> +\t\t// we can read/write sockets_[0]\n> +\t\tclose(sockets_[1]);\n> +\n\nThe child process has a copy of all the memory maps of the parent. I\nthink you'll need to exec() something here (possibly the shim .so\nitself, as a .so can provide a main() function).\n\n> +\t\tdlHandle_ = dlopen(path, RTLD_LAZY);\n> +\t\tif (!dlHandle_) {\n> +\t\t\tstd::cerr << \"Failed to open IPA module: \"\n> +\t\t\t\t << dlerror() << std::endl;\n> +\t\t\treturn -1;\n> +\t\t}\n> +\n> +\t\tvoid *symbol = dlsym(dlHandle_, \"ipaCreate\");\n> +\t\tif (!symbol) {\n> +\t\t\tstd::cerr\n> +\t\t\t\t<< \"Failed to load ipaCreate() from IPA module shared object: \"\n> +\t\t\t\t<< dlerror();\n> +\t\t\tdlclose(dlHandle_);\n> +\t\t\tdlHandle_ = nullptr;\n> +\t\t\treturn -1;\n> +\t\t}\n> +\n> +\t\tIPAIntfFactory ipaCreate = reinterpret_cast(symbol);\n> +\t\tipa_ = std::unique_ptr(ipaCreate());\n\nWhy don't you simple reuse the IPAModule class ?\n\n> +\t\texit(0);\n\nThat's harsh :-) I expect you will need an event loop, which can be\nbased on the EventDispatcher class.\n\n> +\t}\n> +\n> \treturn 0;\n> }\n> \n> +void ShimDummy::cleanup()\n> +{\n> +\tif (dlHandle_)\n> +\t\tdlclose(dlHandle_);\n> +\tdlHandle_ = nullptr;\n> +}\n> +\n> /*\n> * External IPA module interface\n> */","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E8A2D67C0D\n\tfor ;\n\tThu, 6 Jun 2019 12:16:20 +0200 (CEST)","from pendragon.ideasonboard.com (unknown [109.132.30.162])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 78B5733B;\n\tThu, 6 Jun 2019 12:16:20 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1559816180;\n\tbh=Rp0p7TYFqOd33Cw/+DlIZsnxjfUsdnMJGClCZjcOrcU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=CvRWiPJBZu0IsojklQq0dTuSI8mouphtJ/97gEfIGXSrBC6DJDw1J9J5H+BqX0Wzh\n\t+2nwGY8gXju1OBuXL33Nv4GSS5cMOWLf32bzLqyeNzJODiSfTwZKOV0qksrvP7CvLA\n\t6ihpkVS3ZvR+0X3a5zom1o1vzZnB/JPh0zmyUd9U=","Date":"Thu, 6 Jun 2019 13:16:06 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190606101606.GG12825@pendragon.ideasonboard.com>","References":"<20190605221817.966-1-paul.elder@ideasonboard.com>\n\t<20190605221817.966-11-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190605221817.966-11-paul.elder@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [RFC PATCH 10/10] libcamera: ipa: shim: load\n\tIPA module into an IPAInterface","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":"Thu, 06 Jun 2019 10:16:21 -0000"}}]