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

[Cyrill Gorcunov]

On Wed, Jul 18, 2012 at 06:58:26AM +0400, Pavel Emelyanov wrote:
> > 
> > -       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;
> 

And here we need to release data allocated for SkOptsEntry
in pb_dump_sk_opts, not really elegant I think.

> > +       required bytes                  name            = 11;
> 
> Need a comment here saying why this is not a string.

ok

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

indeed

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

Why it's not correct? The number of bytes allocated for SkOptsEntry is known
from SkOptsEntry::n_so_snd_tmo, but sure I'll make it this way.

	Cyrill