[CRIU] Re: [PATCH 2/3] protobuf: Introduce base message prototypes
Stanislav Kinsbursky
skinsbursky at parallels.com
Wed Jul 4 04:58:29 EDT 2012
04.07.2012 11:37, Cyrill Gorcunov пишет:
> This patch introduces base message prototypes Google's protobuf facility.
>
> A short story -- there were a long conversation on which format should
> be used to keep checkpointed data on disk image. We ended up in using
> Google's Protocol Buffers (seehttps://developers.google.com/protocol-buffers/
> for detailed description). Thus every image snippet should be convered
> to use this facility.
>
> But before anything else we need a scaffold, ie .proto files which declare
> entries on disk. This patch does exactly that.
>
> Still we need some tricks to be used for our parasite c/r code -- this code
> can't use libc neither any else library so we provide two helpers
>
> - protobuf_page_write
> - protobuf_page_read
>
> they do write/read page entries on/from disk in protobuf format and can be used
> in pie code.
>
> Note that there is no real use of this facility in our code yet.
>
> Build note: one should have protobuf and protobuf-c installed to be able
> to build crtools.
>
> -http://code.google.com/p/protobuf/
> -http://code.google.com/p/protobuf-c/
>
> Inspired-by: Kinsbursky Stanislav<skinsbursky at openvz.org>
> Inspired-by: Pavel Emelianov<xemul at parallels.com>
> Signed-off-by: Cyrill Gorcunov<gorcunov at openvz.org>
> ---
> Makefile | 14 ++-
> include/page-protobuf-engine.h | 152 +++++++++++++++++++
> protobuf/Makefile | 53 +++++++
> protobuf/image.page.proto | 11 ++
> protobuf/image.proto | 316 ++++++++++++++++++++++++++++++++++++++++
> protobuf/image.x86-64.proto | 51 +++++++
> 6 files changed, 593 insertions(+), 4 deletions(-)
> create mode 100644 include/page-protobuf-engine.h
> create mode 100644 protobuf/Makefile
> create mode 100644 protobuf/image.page.proto
> create mode 100644 protobuf/image.proto
> create mode 100644 protobuf/image.x86-64.proto
>
>
> 0002-protobuf-Introduce-base-message-prototypes.patch
>
>
> diff --git a/Makefile b/Makefile
> index 81c9ca8..db2e6f7 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -3,7 +3,7 @@ include Makefile.inc
> CFLAGS += -I./include
> CFLAGS += -O0 -ggdb3
>
> -LIBS += -lrt -lpthread
> +LIBS += -lrt -lpthread -lprotobuf-c
>
> DEFINES += -D_FILE_OFFSET_BITS=64
> DEFINES += -D_GNU_SOURCE
> @@ -60,6 +60,8 @@ OBJS += mount.o
> OBJS += inotify.o
> OBJS += pstree.o
>
> +PROTOBUF-LIB := protobuf/protobuf-lib.o
> +
> DEPS := $(patsubst %.o,%.d,$(OBJS))
>
> MAKEFLAGS += --no-print-directory
> @@ -68,13 +70,16 @@ include Makefile.syscall
> include Makefile.pie
>
> .PHONY: all test-legacy zdtm test rebuild clean distclean tags cscope \
> - docs help pie
> + docs help pie protobuf
>
> -all: pie
> +all: protobuf pie
> $(Q) $(MAKE) $(PROGRAM)
>
> pie: $(PIE-GEN)
>
> +protobuf:
> + $(Q) $(MAKE) -C protobuf/ all
> +
> %.o: %.c
> $(E) " CC " $@
> $(Q) $(CC) -c $(CFLAGS) $< -o $@
> @@ -91,7 +96,7 @@ pie: $(PIE-GEN)
> $(E) " DEP " $@
> $(Q) $(CC) -M -MT $@ -MT $(patsubst %.d,%.o,$@) $(CFLAGS) $< -o $@
>
> -$(PROGRAM): $(OBJS) $(SYS-OBJ)
> +$(PROGRAM): $(OBJS) $(SYS-OBJ) $(PROTOBUF-LIB)
> $(E) " LINK " $@
> $(Q) $(CC) $(CFLAGS) $^ $(LIBS) -o $@
>
> @@ -120,6 +125,7 @@ clean: cleanpie cleansyscall
> $(Q) $(RM) -f ./*.bin
> $(Q) $(RM) -f ./$(PROGRAM)
> $(Q) $(RM) -rf ./test/dump/
> + $(Q) $(MAKE) -C protobuf/ clean
> $(Q) $(MAKE) -C test/legacy clean
> $(Q) $(MAKE) -C test/zdtm cleandep
> $(Q) $(MAKE) -C test/zdtm clean
> diff --git a/include/page-protobuf-engine.h b/include/page-protobuf-engine.h
> new file mode 100644
> index 0000000..aa2e659
> --- /dev/null
> +++ b/include/page-protobuf-engine.h
> @@ -0,0 +1,152 @@
> +#ifndef PAGE_PROTOBUF_ENGINE_H__
> +#define PAGE_PROTOBUF_ENGINE_H__
> +
> +#include "types.h"
> +#include "compiler.h"
> +
> +#include "syscall.h"
> +
> +/* Little endian variants */
> +static inline size_t fixed64_pack(u64 value, void *out)
> +{
> + *(u64 *)out = value;
> + return 8;
> +}
> +
> +static inline size_t uint32_pack(u32 value, u8 *out)
> +{
> + size_t ret = 0;
> +
> + if (value >= 0x80) {
> + out[ret++] = value | 0x80;
> + value >>= 7;
> + if (value >= 0x80) {
> + out[ret++] = value | 0x80;
> + value >>= 7;
> + if (value >= 0x80) {
> + out[ret++] = value | 0x80;
> + value >>= 7;
> + if (value >= 0x80) {
> + out[ret++] = value | 0x80;
> + value >>= 7;
> + }
> + }
> + }
> + }
> +
Why not just something like this:
static inline size_t uint32_pack(u32 value, u8 *out)
{
...
while(value >= 0x80) {
out[ret++] = value | 0x80;
value >>= 7;
}
}
?
> + out[ret++] = value;
> +
> + return ret;
> +}
> +
> +static inline size_t uint64_pack(u64 value, u8 *out)
> +{
> + u32 hi = (u32)(value >> 32);
> + u32 lo = (u32)(value >> 0);
> + size_t ret;
> +
> + if (hi == 0)
> + return uint32_pack((u32)lo, out);
> +
> + out[0] = (lo >> 0) | 0x80;
> + out[1] = (lo >> 7) | 0x80;
> + out[2] = (lo >> 14) | 0x80;
> + out[3] = (lo >> 21) | 0x80;
> +
> + if (hi < 8) {
> + out[4] = (hi << 4) | (lo >> 28);
> + return 5;
> + } else {
> + out[4] = ((hi & 7) << 4) | (lo >> 28) | 0x80;
> + hi >>= 3;
> + }
> +
> + ret = 5;
> + while (hi >= 128) {
> + out[ret++] = hi | 0x80;
> + hi >>= 7;
> + }
> + out[ret++] = hi;
> +
> + return ret;
> +}
> +
> +static inline size_t tag_pack(u32 id, u8 *out)
> +{
> + if (id < (1 << (32 - 3)))
> + return uint32_pack(id << 3, out);
> + else
> + return uint64_pack(((u64)id) << 3, out);
> +}
> +
> +#define CR_MAX_UINT64_ENCODED_SIZE 10
> +#define CR_PROTOBUF_TYPE_64BIT 1
> +#define CR_PROTOBUF_TYPE_LENGTH_PREFIXED 2
> +
> +#define CR_PROTOBUF_PAGE_ENTRY_SIZE 4108
> +
> +static __maybe_unused int protobuf_page_write(int fd, unsigned long vaddr)
> +{
> + u8 scratch[CR_MAX_UINT64_ENCODED_SIZE * 2];
> + u8 *page_data = (void *)vaddr;
> + size_t offset, ret;
> +
> + /* virtual address */
> + offset = tag_pack(1, scratch);
> + scratch[0] |= CR_PROTOBUF_TYPE_64BIT;
> + offset += fixed64_pack(*(const u64 *)&vaddr, scratch + offset);
> +
> + ret = sys_write(fd, scratch, offset);
> + if (ret != offset)
> + return -1;
> +
> + /* page bytes stream header */
> + offset = tag_pack(2, scratch);
> + scratch[0] |= PROTOBUF_C_WIRE_TYPE_LENGTH_PREFIXED;
> + offset += uint32_pack(PAGE_SIZE, scratch + offset);
> +
> + ret = sys_write(fd, scratch, offset);
> + if (ret != offset)
> + return -2;
> +
> + /* and finally data stream */
> + ret = sys_write(fd, page_data, PAGE_SIZE);
> + if (ret != PAGE_SIZE)
> + return -3;
> +
> + return 0;
> +}
> +
> +static __maybe_unused int protobuf_page_read(int fd, unsigned long vaddr)
> +{
> + u8 scratch[CR_MAX_UINT64_ENCODED_SIZE * 2];
> + u8 *page_data = (void *)vaddr;
> + size_t offset, ret;
> +
> + /* virtual address */
> + offset = tag_pack(1, scratch);
> + scratch[0] |= CR_PROTOBUF_TYPE_64BIT;
> + offset += fixed64_pack(*(const u64 *)&vaddr, scratch + offset);
> +
> + ret = sys_lseek(fd, offset, SEEK_CUR);
> + if (ret < 0)
> + return -1;
> +
> + /* page bytes stream header */
> + offset = tag_pack(2, scratch);
> + scratch[0] |= PROTOBUF_C_WIRE_TYPE_LENGTH_PREFIXED;
> + offset += uint32_pack(PAGE_SIZE, scratch + offset);
> +
> + ret = sys_lseek(fd, offset, SEEK_CUR);
> + if (ret < 0)
> + return -2;
> +
> + /* and finally data stream */
> + ret = sys_read(fd, page_data, PAGE_SIZE);
> + if (ret != PAGE_SIZE)
> + return -3;
> +
> + return 0;
> +}
> +
First of all, I really hope, that protobuf logic, implemented in the header,
works as expected.
Second, I don't understand, where these protobuf_page_read() and
protobuf_page_write() are used.
Even if it's just a RFC part, it should be in separated patch from my POW.
> +#endif /* PAGE_PROTOBUF_ENGINE_H__ */
> diff --git a/protobuf/Makefile b/protobuf/Makefile
> new file mode 100644
> index 0000000..086da82
> --- /dev/null
> +++ b/protobuf/Makefile
> @@ -0,0 +1,53 @@
> +-include ../Makefile.inc
> +
> +CFLAGS += -I./include
> +CFLAGS += -O0 -ggdb3
> +
> +DEFINES += -D_FILE_OFFSET_BITS=64
> +DEFINES += -D_GNU_SOURCE
> +
> +ifneq ($(WERROR),0)
> + WARNINGS += -Werror
> +endif
> +
> +ifeq ($(DEBUG),1)
> + DEFINES += -DCR_DEBUG
> +endif
> +
> +WARNINGS += -Wall
> +CFLAGS += $(WARNINGS) $(DEFINES)
> +
> +LIBRARY := protobuf-lib.o
> +
> +PROTO_FILES += image.x86-64.proto
> +PROTO_FILES += image.page.proto
> +PROTO_FILES += image.proto
> +
> +HDRS := $(patsubst %.proto,%.pb-c.h,$(PROTO_FILES))
> +SRCS := $(patsubst %.proto,%.pb-c.c,$(PROTO_FILES))
> +OBJS := $(patsubst %.c,%.o,$(SRCS))
> +
> +.DEFAULT_GOAL := all
> +
> +%.pb-c.c: %.proto
> + $(E) " PROTOBUF "$@
> + $(Q) protoc-c --c_out=./ $<
> +
> +%.o: %.c
> + $(E) " CC "$@
> + $(Q) $(CC) -c $(CFLAGS) $< -o $@
> +
> +.SECONDARY:
> +
> +$(LIBRARY): $(OBJS)
> + $(E) " LINK "$@
> + $(Q) ld -r -o $@ $(OBJS)
> +
> +.PHONY: all clean
> +
> +all: $(LIBRARY)
> +
> +clean:
> + $(E) " CLEAN PROTOBUF"
> + $(Q) rm -f $(SRCS) $(HDRS) $(OBJS) $(LIBRARY)
> +
Could you clarify, did you update somehow my patches for Makefiles or not?
> diff --git a/protobuf/image.page.proto b/protobuf/image.page.proto
> new file mode 100644
> index 0000000..1997d71
> --- /dev/null
> +++ b/protobuf/image.page.proto
> @@ -0,0 +1,11 @@
> +/*
> + * ATTENTION
> + *
> + * If you are to change it make sure parasite code
> + * protobuf-engine is updated.
> + */
> +
> +message page_entry {
> + required fixed64 va = 1;
> + required bytes data = 2;
> +}
The same here - I can't find, where it's used.
> diff --git a/protobuf/image.proto b/protobuf/image.proto
> new file mode 100644
> index 0000000..edd85f2
> --- /dev/null
> +++ b/protobuf/image.proto
> @@ -0,0 +1,316 @@
> ...
> +
> +message fdinfo_entry {
> + required uint32 fd = 1;
> + required uint32 type = 2;
> + required uint32 flags = 3;
> + required uint32 id = 4;
> +}
> +
This one looks nice, but I don't see where do you remove struct fdinfo_entry
from our images header.
The others, as I said, should come with it's use and remove of it's previous
implementation - it's much easier to review patches, organized this way.
--
Best regards,
Stanislav Kinsbursky
More information about the CRIU
mailing list