[CRIU] Re: [PATCH 4/5] util-net: Add send_fds and recv_fds

[Cyrill Gorcunov]

On Wed, Mar 21, 2012 at 05:09:57PM +0400, Pavel Emelyanov wrote:
...
> > +static int *scm_fdset_init(struct scm_fdset *fdset, struct sockaddr_un *saddr, int saddr_len)
> > +{
> > +	struct cmsghdr *cmsg;
> > +
> > +	BUILD_BUG_ON(CR_SCM_MAX_FD > SCM_MAX_FD);
> > +	BUILD_BUG_ON(sizeof(fdset->msg_buf) < (CMSG_SPACE(sizeof(int) * CR_SCM_MAX_FD)));
> > +
> > +	fdset->nr_fds			= CR_SCM_MAX_FD;
> 
> This field is effectively constant is this code. Remove it.

Why? I don't get it. Via this member we set up active number
of fds to be send or, in case of receiving fds, we set up the
number of fds obtained here. At moment of initialization we
should not leave this member untouched and set it to max
value pointing that the whole array can be used.

Sure I can simply drop this line, but IMHO, it's wrong.
...

> > +
> > +	scm_fdset_init_chunk(fdset, CR_SCM_MAX_FD);
> 
> Already done in caller.

OK (frankly I prefer explicit scm_fdset_init_chunk here,
since it's just a couple of instruction but as you wish ;)

> > +
> > +	min_fd = (cmsg->cmsg_len - sizeof(struct cmsghdr)) / sizeof(int);
> > +	min_fd = min(min_fd, CR_SCM_MAX_FD);
> 
> This is wrong. If the peer has sent us (for any reason) more fds than we expected
> we should report an error, not silently trim the array.

The kernel do check for space before it passes data to user-space, which means
we never get more than CR_SCM_MAX_FD here. Probably I should simply add

	BUG_ON(min_fd > CR_SCM_MAX_FD);

instead?

> > +
> > +	for (i = 0; i < nr_fds; i += fdset.nr_fds) {
> > +		scm_fdset_init_chunk(&fdset, nr_fds - i);
> 
> The trim insite the _init_cunk is obfuscating.


OK, I'll add

	int left = nr_fds - i;
	scm_fdset_init_chunk(&fdset, left);

will you be fine with that?

	Cyrill