[CRIU] Re: [PATCH 2/3] protobuf: Introduce base message prototypes

[Cyrill Gorcunov]

On Wed, Jul 04, 2012 at 12:58:29PM +0400, Stanislav Kinsbursky wrote:
...
> >+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;
> 	}
> }
> 
> ?

I took existing implementation from protobuf-c,
sure it can be optimized this way, but at moment
of testing I needed 1:1 map. Will update.

> >+	return 0;
> >+}
> >+
> 
> 
> First of all, I really hope, that protobuf logic, implemented in the
> header, works as expected.

Yes, it does I've tested it a lot before posting out.

> Second, I don't understand, where these protobuf_page_read() and
> protobuf_page_write() are used.

It'll be used in restore.c/parasite.c code where we read and write
page data. This helpers allow us to not use protobuf library in
our pie code but operate directly with PB abi.

> Even if it's just a RFC part, it should be in separated patch from my POW.

Yes, my bad. Sorry.

> >+
> >+clean:
> >+	$(E) "  CLEAN PROTOBUF"
> >+	$(Q) rm -f $(SRCS) $(HDRS) $(OBJS) $(LIBRARY)
> >+
> 
> 
> Could you clarify, did you update somehow my patches for Makefiles or not?

Nope, I took them directly from the patch you've sent me.

> >+
> >+message page_entry {
> >+	required fixed64			va			= 1;
> >+	required bytes				data			= 2;
> >+}
> 
> The same here - I can't find, where it's used.

it's unused at moment, my bad, soory.

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

Yes, I'm updating my repo, will send another series a bit later. Thanks!

	Cyrill