[libcamera-devel] [PATCH v2 3/3] test: ipc: unix: Add test for IPCUnixSocket

[Niklas Söderlund]

Hi Laurent,

Thanks for your feedback.

On 2019-07-01 13:15:29 +0300, Laurent Pinchart wrote:
> > >> +	int prepareFDs(IPCUnixSocket::Payload *message, unsigned 
> > >> int num)
> > >> +	{
> > >> +		int fd = open("/proc/self/exe", O_RDONLY);
> > >> +		if (fd < 0)
> > >> +			return fd;
> > >> +
> > >> +		int size = 0;
> > >> +		for (unsigned int i = 0; i < num; i++) {
> > >> +			int clone = dup(fd);
> > >> +			if (clone < 0)
> > >> +				return clone;
> > >> +
> > >> +			size += calculateLength(clone);
> > >> +			message->fds.push_back(clone);
> > >> +		}
> > > 
> > > I still think it would be nicer to use mkstemp() to create temporary
> > > files, and test their content instead of just the size. You could then
> > > drop one of the two fd-related tests, and for instance create 2
> > > temporary files with data, send their fds, and receive back the fd of a
> > > new temporary file that contains the concatenation of the two.
> > 
> > I think this one is nicer :-)
> > 
> > I'm quiet confident that file IO works in the system, what I'm not so 
> > confident in is that the IPC mechanism is able to pass multiple FDs.  
> > With this construct we test passing 2 and 7 file descriptors and verify 
> > that the FDs are valid by the most simple operation on them. Adding a 
> > test which you describe would indeed test a similar thing but IMHO in a 
> > more complex way with no benefit to the IPC verification.
> 
> My concern here is that by passing multiple fds refering to the same
> file, you may indeed be testing passing multiple fds, but you can't
> verify the ordering :-S

This is a reason I can agree with. I will add a test to verify ordering 
of fds, but I will keep the use of dup() here as it tests transferring 
may fds.

> 
> > Also as soon as we start creating temporary files in a TC we also need 
> > to care more about cleanup logic. As we don't wish to leave waste after 
> > us, with this construct it's not an issue.
> 
> I was thinking you could unlink the file as soon as it gets created, but
> another option would be to use shm_open() to avoid creating files on
> disk.

Nice tip, I will try to make use of shm_open() in the next version for 
the additional test.

-- 
Regards,
Niklas Söderlund