[libcamera-devel] [RFC PATCH 10/10] libcamera: ipa: shim: load IPA module into an IPAInterface

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Wed, Jun 05, 2019 at 06:18:17PM -0400, Paul Elder wrote:
> Implement the dummy shim's init method that takes a path for the IPA
> module to be loaded. Set up IPC, then fork and load the IPAInterface
> implementation from the IPA module shared object.
> 
> Implement a cleanup method along with it.
> 
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>

I understand you've kept this patch separate as it's a bit of an early
draft, but I think it should be squashed with the patch that adds the
dummy shim, as it makes little sense to have a dummy implementation that
does nothing. I would also rename it to something else than dummy, may
-linux-default or something similar ?


> ---
> This is the most draft-y patch of the whole series. I wasn't sure how to
> put what kind of IPC, so I kind of just did this very skeletal "give you
> an idea" type of IPC initialization. Privilege dropping will be looked
> into for the next version.
> 
>  src/ipa/shim_dummy.cpp | 79 ++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 77 insertions(+), 2 deletions(-)
> 
> diff --git a/src/ipa/shim_dummy.cpp b/src/ipa/shim_dummy.cpp
> index 4d28c2d..f2e6961 100644
> --- a/src/ipa/shim_dummy.cpp
> +++ b/src/ipa/shim_dummy.cpp
> @@ -6,6 +6,14 @@
>   */
>  
>  #include <iostream>
> +#include <memory>
> +
> +#include <dlfcn.h>
> +#include <string.h>
> +#include <stdlib.h>
> +#include <sys/socket.h>
> +#include <sys/types.h>
> +#include <unistd.h>
>  
>  #include <libcamera/ipa/ipa_interface.h>
>  #include <libcamera/ipa/ipa_module_info.h>
> @@ -17,20 +25,87 @@ class ShimDummy : public IPAInterface
>  public:
>  	int init();
>  	int init(const char *path);
> +
> +	void cleanup();
> +
> +private:
> +	std::unique_ptr<IPAInterface> ipa_;
> +
> +	int sockets_[2];
> +	int childPid_;
> +	void *dlHandle_;
> +
> +	typedef IPAInterface *(*IPAIntfFactory)(void);
>  };
>  
>  int ShimDummy::init()
>  {
> -	std::cout << "okay shim init without path" << std::endl;
> +	std::cout << "initializing IPA via dummy shim!" << std::endl;
>  	return 0;
>  }
>  
>  int ShimDummy::init(const char *path)
>  {
> -	std::cout << "initializing dummy shim!" << std::endl;
> +	std::cout << "initializing dummy shim! loading IPA from " << path
> +		  << std::endl;
> +
> +	/* We know the IPA module is valid, otherwise IPAManager wouldn't
> +	 * even give its path to us. */
> +
> +	// setup comm
> +	if (socketpair(AF_UNIX, SOCK_STREAM, 0, sockets_)) {
> +		int err = errno;
> +		std::cerr << "Error opening sockets: " << strerror(errno)
> +			  << std::endl;
> +		return err;
> +	}
> +
> +	// fork

I would separate IPC and process management in different functions, as
both will grow. I even think you should have separate classes to handle
those.

One item that needs to be researched is how to handle all the file
descriptors created by the application that should not passed to the
child process.

> +	if ((childPid_ = fork()) == -1) {
> +		int err = errno;
> +		std::cerr << "Failed to fork: " << strerror(errno) << std::endl;
> +		return err;
> +	} else if (childPid_) {
> +		// we are parent
> +		// we can read/write sockets_[1]
> +		close(sockets_[0]);
> +	} else {
> +		// we are child
> +		// we can read/write sockets_[0]
> +		close(sockets_[1]);
> +

The child process has a copy of all the memory maps of the parent. I
think you'll need to exec() something here (possibly the shim .so
itself, as a .so can provide a main() function).

> +		dlHandle_ = dlopen(path, RTLD_LAZY);
> +		if (!dlHandle_) {
> +			std::cerr << "Failed to open IPA module: "
> +				  << dlerror() << std::endl;
> +			return -1;
> +		}
> +
> +		void *symbol = dlsym(dlHandle_, "ipaCreate");
> +		if (!symbol) {
> +			std::cerr
> +				<< "Failed to load ipaCreate() from IPA module shared object: "
> +				<< dlerror();
> +			dlclose(dlHandle_);
> +			dlHandle_ = nullptr;
> +			return -1;
> +		}
> +
> +		IPAIntfFactory ipaCreate = reinterpret_cast<IPAIntfFactory>(symbol);
> +		ipa_ = std::unique_ptr<IPAInterface>(ipaCreate());

Why don't you simple reuse the IPAModule class ?

> +		exit(0);

That's harsh :-) I expect you will need an event loop, which can be
based on the EventDispatcher class.

> +	}
> +
>  	return 0;
>  }
>  
> +void ShimDummy::cleanup()
> +{
> +	if (dlHandle_)
> +		dlclose(dlHandle_);
> +	dlHandle_ = nullptr;
> +}
> +
>  /*
>   * External IPA module interface
>   */

-- 
Regards,

Laurent Pinchart