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

[Pavel Emelyanov]

On 07/17/2012 04:10 PM, Cyrill Gorcunov wrote:
> On Tue, Jul 17, 2012 at 02:21:27PM +0400, Cyrill Gorcunov wrote:
>> On Tue, Jul 17, 2012 at 02:18:26PM +0400, Pavel Emelyanov wrote:
>>>>>
>>>>> 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.
>>
>> Unused functions lead us to the state where gcc complains on them.
>> Anyway, thanks for comments, Pavel, i'll try to address all complains.
> 
> Pavel, could you please check if these two patches looks good for you?
> 
> 	Cyrill
> 
> @@ -120,15 +123,24 @@ static int dump_one_unix_fd(int lfd, u32 id, const struct fd_parms *p)
> 
>         BUG_ON(sk->sd.already_dumped);
> 
> +       skopts = alloc_socket_opts();
> +       if (!skopts)
> +               goto err;
> +
> +       pb_prep_fown(&fown, &p->fown);
> +
> +       ue.name.len     = (size_t)sk->namelen;
> +       ue.name.data    = (void *)sk->name;
> +
>         ue.id           = id;
>         ue.ino          = sk->sd.ino;
>         ue.type         = sk->type;
>         ue.state        = sk->state;
> -       ue.namelen      = sk->namelen;
>         ue.flags        = p->flags;
>         ue.backlog      = sk->wqlen;
>         ue.peer         = sk->peer_ino;
> -       ue.fown         = p->fown;
> +       ue.fown         = &fown;
> +       ue.opts         = skopts;
>         ue.uflags       = 0;
> 
>         if (ue.peer) {
> @@ -190,12 +202,10 @@ static int dump_one_unix_fd(int lfd, u32 id, const struct fd_parms *p)
>                                 ue.ino, ue.peer);
>         }
> 
> -       if (dump_socket_opts(lfd, &ue.opts))
> +       if (pb_dump_socket_opts(lfd, skopts))
>                 goto err;
> 

The above can be simplified as

UnixSkEntry ue = ..._INIT;
SkOptsEntry opts = ..._INIT;

ue.foo = foo;
ue.bar = bar;
ue.opts = &opts;

if (pb_dump_sk_opts(&opts)) /* allocates and dumps */
	goto err;

if (pb_write())
	goto err;

return 0;

> +message unix_sk_entry {
> +       required uint32                 id              =  1;
> +       required uint32                 ino             =  2;
> +       required uint32                 type            =  3;
> +       required uint32                 state           =  4;
> +       required uint32                 flags           =  5;
> +       required uint32                 uflags          =  6;
> +       required uint32                 backlog         =  7;
> +       required uint32                 peer            =  8;
> +       required fown_entry             fown            =  9;
> +       required sk_opts_entry          opts            = 10;
> +       required bytes                  name            = 11;

Need a comment here saying why this is not a string.

> +}

> Note, if ui->ue->name.len then ui->name = NULL will be set this
> way by PB library upon reading.

No, the ui->ue->name.data will be set to NULL, not the ui->name.
> 
> +int pb_dump_socket_opts(int sk, SkOptsEntry *soe)
> +{
> +       int ret = 0;
> +
> +       if (soe->n_so_snd_tmo != 2 || soe->n_so_rcv_tmo != 2)
> +               return -1;
> +
> +       ret |= dump_opt(sk, SO_SNDBUF, &soe->so_sndbuf);
> +       ret |= dump_opt(sk, SO_RCVBUF, &soe->so_rcvbuf);
> +
> +       ret |= do_dump_opt(sk, SO_SNDTIMEO, soe->so_snd_tmo, pb_repeated_size(soe, so_snd_tmo));
> +       ret |= do_dump_opt(sk, SO_RCVTIMEO, soe->so_rcv_tmo, pb_repeated_size(soe, so_rcv_tmo));

Using pb_repeated_size is not correct here. Copy data on stack and use _this_ size.
The same applies to pb_restore_socket_opts.

> +
> +       return ret;
> +}

Thanks,
Pavel