[CRIU] Re: [PATCH 3/4] protbuf: Add Protocol Buffers (PB) helpers v2

[Pavel Emelyanov]

On 07/06/2012 07:32 PM, Cyrill Gorcunov wrote:
> 
> To not bloat util.h or image.h the protobuf.h introduced to
> handle all PB needs.
> 
> We represent our PB objects as pairs of records
> 
>   | 4 bytes for object packed size
>   +---
>   | X bytes -- object itself
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov at openvz.org>
> ---
>  Makefile           |    1 +
>  include/protobuf.h |   49 ++++++++++++++++++++++++++++++++++
>  protobuf.c         |   75 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 125 insertions(+), 0 deletions(-)
>  create mode 100644 include/protobuf.h
>  create mode 100644 protobuf.c
> 

> +int pb_read_object_with_header(int fd, void **pobj, pb_unpack_t *unpack, bool eof)
> +{
> +	void *buf = NULL;
> +	int ret = -1;
> +	u32 size;
> +
> +	ret = read_img_eof(fd, &size);

This all is great, but I'd insist you stop using the read/write_img helpers since
ally they were required for was to factor out eof/errors handling and error message
report between callers. With pb_read/write helpers we can push the pr_err right
here and use plain read/write.

At the very end all the read/write_img should be removed.

> +	if (ret == 0) {
> +		if (eof) {
> +			*pobj = NULL;
> +			return 0;
> +		} else {
> +			pr_err("Unexpected EOF\n");
> +			return -1;
> +		}
> +	} else if (ret < 0)
> +		return -1;
> +
> +	ret = -1;
> +	buf = xmalloc(size);
> +	if (!buf)
> +		goto err;

Just a TODO: have a static/onstack variable of e.g. 512 bytes and put the
image bits _there_ if they fit. This should improve the performance a bit.

> +	ret = read_img_buf(fd, buf, size);
> +	if (ret < 0)
> +		goto err;
> +
> +	*pobj = (void *)unpack(NULL, size, buf);
> +	if (!*pobj) {
> +		ret = -1;
> +		pr_err("Failed unpacking object %p\n", pobj);
> +		goto err;
> +	}
> +err:
> +	xfree(buf);
> +	return ret;
> +}
> +
> +int pb_write_object_with_header(int fd, void *obj, pb_getpksize_t *getpksize, pb_pack_t *pack)
> +{
> +	u32 size, packed;
> +	void *buf = NULL;
> +	int ret = -1;
> +
> +	size = getpksize(obj);
> +	buf = xmalloc(size);
> +	if (!buf)
> +		goto err;
> +
> +	packed = pack(obj, buf);
> +	if (packed != size) {
> +		pr_err("Packing object %p failed\n", obj);
> +		goto err;
> +	}
> +
> +	ret = write_img(fd, &size);
> +	if (!ret)
> +		ret = write_img_buf(fd, buf, size);
> +err:
> +	xfree(buf);
> +	return ret;
> +}