[CRIU] Re: [RFC] fds TX/RX

[Cyrill Gorcunov]

On Fri, Mar 16, 2012 at 08:36:36PM +0400, Pavel Emelyanov wrote:
> The patch was corrupted by mailserver. Big patches should be sent as
> attachments to avoid this.

Ouch, sorry about that.

> > +       unsigned int            scm_fd_locl;    /* our fd number transferred from parasite */
> > +       unsigned int            scm_fd_orig;    /* corresponding original fd */
> 
> Simple names like fd and lfd are more than enough.

OK.

> > +       struct vma_entry        *vma_entry;     /* Used for VMA file maps */
> 
> What does this vma_entry do with this patch?

The VMA is used to obtain file later in patch

dump_one_reg_file
	if (params->type == FDINFO_MAP)
		e.addr	= params->vma_entry->start;
	else
		e.addr	= params->scm_fd_orig;

I'll check if I manage somehow simplify this. Pavel, you should note
that this patch is an RFC and I wanted to present something which works
and if you would look at the resulting code afther the patch applied you
will see the separation of functions in files and structures really well
done and handled easily.

But sure it's my _bad_ that I squashed everything into one big patch
while it should be done in easy-reviewable series. Sorry about that,
really.

> > +static int dump_one_reg_file(const struct fd_parms *params,
> > +                            const struct cr_fdset *cr_fdset)
> 
> If you add or remove args from fn don't rename other in the same patch.

ok

> > 
> >         /* Dump /proc/pid/cwd */
> >         params = (struct fd_parms) {
> > +               .scm_fd_orig    = fd,
> 
> Should be some invalid value.
> 

ok, thanks

> > +       /*
> > +        * Check if it's a socket.
> > +        */
> > +       {
> 
> Putting a { } block into the middle of a function is reprehensible.
> Don't do this again, please.

I thought about adding some helper here instead. Sure will cleanup.

> > +       ret = parasite_tx_fds_seized(ctl, fds_map->fd_orig, fds_map->fd_locl, fds_map->nr_fds);
> 
> Use word "drain" for anything related to getting fds from parasite.
> In particular this fn should be called parasaite_drain_fds_seized.

ok, thanks

> Plz, just patch the read_fd_parms fn for reading pos, flags, etc in a new manner.

sure

> > +int fds_scm_map_expand(struct fds_scm_map *map)
> 
> Have you ever seen an app holding more than 1024 descriptors open? Me not.
> Plz, just allocate a single page and abort dumping if it's not enough.

Sure this will even simplify all the concept (but I shiver to imagine
one day "me not" become no excuse).

> > +int fds_scm_collect_orig(pid_t pid, struct fds_scm_map **map)
> 
> This code has nothing to do with scm.

It does collect fds for SCM tranfer, it does put them into a structure
which do track the draining procedure, so I really fail to see how it's
_not_ related to SCM. But sure, if you wish I'll drop scm here.

> > +extern struct fds_scm_map *fds_scm_map_create(void);
> 
> static
> 

ok

> size_fd_locl == size_fd_orig = ROUND_UP(nr_fds * sizeof(int), PAGE_SIZE)
> 
> IOW -- you only need one variable.
> 

yup, because of fixed memory allocation. Actually in first place (when there
were no a big hammer as PAGE_SIZE for all /and I still believe this fixed
size one day become pain in ass for us, I'm sure ;)/ I thought about simply
interleaving fds in one big slab of memory and use some math to calculate
position, but then I though this will be useless complication. So I turned
into two variables.

Sure I'll simply. Thanks.

> > +};
> > +
> > +#define SCM_MAP_NEED_EXPAND(size, nr)  (((size) / sizeof(int)) <= (nr))
> 
> Local to respective .c file
> The macros name is longer than the code it covers -> useless

OK, but frankly "long macro name" is not an argument though ;)

> > -       ret = parasite_set_logfd(ctl, pid);
> > +       ret = parasite_set_logfd(ctl);
> 
> No cleanups in the middle of the big patch.
> I'm sick and tired of reminding this again and again.
> 

My bad, Pavel, sorry!

> > 
> > +static int builtin_atoi(char *str)
> 
> We removed this one several days ago and you don't use it in this patch...

Yeah, rudiment.

> > +#if 1
> > +       sys_write_msg("\tNRfds: ");
> > +       sys_write_msg(long2hex(fdset->__nr_fds));
> > +       sys_write_msg("\n");
> > +
> > +       {
> > +               int i;
> > +               int *fd = scm_fdset_first(fdset);
> > +               for (i = 0; i < fdset->__nr_fds; i++) {
> > +                       sys_write_msg("\tTX:    ");
> > +                       sys_write_msg(long2hex(fd[i]));
> > +                       sys_write_msg("\n");
> > +               }
> > +       }
> > +#endif
> 
> What's this? A garbage?

No, this is debug code, it prints out which exactly FDS were
passed to host side. Unf. we don't have such things as 'log-level'
in parasite code and I was unable to use pr_debug or something.
I'll drop it.

> 
> > +       cmsg = CMSG_FIRSTHDR(&fdset->hdr);
> > +       if (!cmsg || cmsg->cmsg_type != SCM_RIGHTS)
> > +               return -EINVAL;
> > +
> > +       min_fd = (cmsg->cmsg_len - sizeof(struct cmsghdr)) / sizeof(int);
> > +       min_fd = min(min_fd, CR_SCM_MAX_FD);
> > +
> > +       fdset->nr_fds_rx = min_fd;
> > +
> > +       return 0;
> > +}
> > +
> 
> The above is beyond good and evil, sorry :(
> 
> Just expand the existing send_fd and recv_fd to work on arbitrary set of fds and
> make the former call theirs new versions with an array of lengh 1.
> 
> And send this in a separate patch.

Sure. Thanks a lot for comments, Pavel!

	Cyrill