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

[Pavel Emelyanov]

On 07/17/2012 02:09 PM, Cyrill Gorcunov wrote:
> 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?

Yes please.

>>
>>> +	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).

Allocate can happen here, fill should be in the opt dumping fn.

>>> @@ -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.

And this is wrong -- ub->name will be uninitialized in the (removed) else branch.

>>>  		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.

Helpers and .proto file in one patch users in 2 others.

> 	Cyrill
> .
>