[CRIU] Re: [PATCH 6/6] protobuf: Convert sk_packet_entry to PB engine

[Cyrill Gorcunov]

On Tue, Jul 17, 2012 at 01:58:44PM +0400, Pavel Emelyanov wrote:
> On 07/13/2012 09:05 PM, Cyrill Gorcunov wrote:
> > 
> > Signed-off-by: Cyrill Gorcunov <gorcunov at openvz.org>
> > ---
> >  include/image.h          |    6 ---
> >  protobuf/Makefile        |    1 +
> >  protobuf/sk-packet.proto |    4 ++
> >  sk-queue.c               |   77 +++++++++++++++++++++++++--------------------
> >  4 files changed, 48 insertions(+), 40 deletions(-)
> >  create mode 100644 protobuf/sk-packet.proto
> > 
> 
> > @@ -120,15 +123,32 @@ static int dump_one_unix_fd(int lfd, u32 id, const struct fd_parms *p)
> >  
> >  	BUG_ON(sk->sd.already_dumped);
> >  
> > +	fown.uid	= p->fown.uid;
> > +	fown.euid	= p->fown.euid;
> > +	fown.signum	= p->fown.signum;
> > +	fown.pid_type	= p->fown.pid_type;
> > +	fown.pid	= p->fown.pid;
> 
> You have helper for that already, don't you?

I've dropped all fown conversion in patch "protobuf: Drop fown_t type".
And yeah, there were pb_ helper for this earlier, should I update?

> 
> > +	skopts.n_so_rcv_tmo	= 2;
> > +	skopts.n_so_snd_tmo	= 2;
> 
> 2nd attempt: these should go to sockopt dumping code.

Wait, this member is referenced below in ue entry

	ue.opts		= &skopts;

thus is better to be allocated before assignment I think.
Maybe I should add some alloc_skopt() helper instead of
open-coding here? (I don't like the idea of allocating this
entries in sockopt dumping code, this look wrong for me,
because it's vague move I think).

> > @@ -766,16 +776,18 @@ int collect_unix_sockets(void)
> >  			if (ui->name[0] != '\0' &&
> >  			    !(ui->ue->uflags & USK_EXTERN))
> >  				unlink(ui->name);
> > -		} else
> > -			ui->name = NULL;
> > +		}
> 
> Wrong. ui->name is not NULL after xmalloc.

This is removed lines.

> >  		pr_info(" `- Got 0x%x peer 0x%x\n", ui->ue->ino, ui->ue->peer);
> >  		file_desc_add(&ui->d, ui->ue->id, &unix_desc_ops);
> >  		list_add_tail(&ui->list, &unix_sockets);
> > +
> > +		ui = NULL;
> 
> What for?

yeah, redundant, thanks.

> > +int pb_restore_socket_opts(int sk, SkOptsEntry *soe)
> > +{
> > +	int ret = 0;
> > +
> > +	ret |= restore_opt(sk, SO_SNDBUFFORCE, &soe->so_sndbuf);
> > +	ret |= restore_opt(sk, SO_RCVBUFFORCE, &soe->so_rcvbuf);
> > +
> > +	if (soe->n_so_snd_tmo > 1)
> 
> if != 2 report error

ok

> 
> > +		ret |= do_restore_opt(sk, SO_SNDTIMEO, soe->so_snd_tmo,
> > +				      pb_repeated_size(soe, so_snd_tmo));
> > +	else
> > +		ret |= -1;
> > +	if (soe->n_so_rcv_tmo > 1)
> > +		ret |= do_restore_opt(sk, SO_RCVTIMEO, soe->so_rcv_tmo,
> > +				      pb_repeated_size(soe, so_rcv_tmo));
> > +	else
> > +		ret |= -1;
> > +
> > +	return ret;
> > +}
> 
> options dump/restore -- should be separate patch

How? They are part of sk-unix.proto.

	Cyrill