[CRIU] Re: [PATCH 8/8] protobuf: Convert unix_sk_entry to PB engine

[Cyrill Gorcunov]

On Tue, Jul 17, 2012 at 07:41:11AM +0400, Pavel Emelyanov wrote:
> On 07/12/2012 05:56 PM, Cyrill Gorcunov wrote:
> > 
> > Signed-off-by: Cyrill Gorcunov <gorcunov at openvz.org>
> > ---
> >  include/image.h        |   16 ------
> >  include/sockets.h      |    7 +++
> >  protobuf/Makefile      |    2 +
> >  protobuf/sk-opts.proto |    7 +++
> >  protobuf/unix-sk.proto |   17 +++++++
> >  sk-unix.c              |  124 ++++++++++++++++++++++++++----------------------
> >  sockets.c              |   57 ++++++++++++++++++++++
> >  7 files changed, 158 insertions(+), 72 deletions(-)
> >  create mode 100644 protobuf/sk-opts.proto
> >  create mode 100644 protobuf/unix-sk.proto
> 
> The whole name/namelen management here looks wrong.

namelen is used to copy name to socket, ie say here

static int bind_unix_sk(int sk, struct unix_sk_info *ui)
{
	struct sockaddr_un addr;
	...
	memset(&addr, 0, sizeof(addr));
	addr.sun_family = AF_UNIX;
	memcpy(&addr.sun_path, ui->name, ui->ue->namelen);
	...
}

> > -			ui->name = xmalloc(ui->ue->namelen);
> > -			if (ui->name == NULL)
> > +			if (!ui->ue->name) {
> > +				pr_err("Empty unix name but len %d\n", ui->ue->namelen);
> >  				break;
> > +			}
> 
> This can't be so. The name is defined as required at protofile (which is not so, BTW)
> thus parser should warn.

yeah, this is redundant

> 
> >  	ue.namelen	= sk->namelen;
> > +	ue.name		= sk->name;
> 
> How does this work for abstract names starting with '\0'?

namelen = 0 and name != nil

> 
> /methinks that:
> 
> The namelen param looks excessive. First, the protobuf name field should be optional.

ok

> Next, it should be not a string but rather a byte sequence at least for abstract
> names (since they start with '\0' and may contain '\0'-s in the middle :( )

ah, ok

> 
> I'd do it like:
> 
> declare name as optional string
> add yet another flag saying that a name is an abstract one
> check on dump that abstract names do not have '\0'-s in the middle
> on restore add leading '\0' if the flag is set
> don't inherit the namelen in proto file

hmm, ok, will try it this way, thanks!

	Cyrill