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

[Pavel Emelyanov]

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.

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

>  	ue.namelen	= sk->namelen;
> +	ue.name		= sk->name;

How does this work for abstract names starting with '\0'?

/methinks that:

The namelen param looks excessive. First, the protobuf name field should be optional.
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 :( )

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

Thanks,
Pavel