[CRIU] Re: [PATCH 8/8] protobuf: Convert IPC entries to PB format

[Cyrill Gorcunov]

On Wed, Jul 18, 2012 at 02:16:55PM +0400, Stanislav Kinsbursky wrote:
> >
> >+#include "protobuf.h"
> >+#include "protobuf/ipc-var.pb-c.h"
> >+#include "protobuf/ipc-desc.pb-c.h"
> 
> Does this include really required?IpcMsg msg = IPC_MSG__IN

Yeah, the ipc-desc.pb-c.h is included implicilty in other headers,
we can drop it.

> >
> >  static int dump_ipc_var(int fd)
> >  {
> >-	int ret;
> >-	struct ipc_var_entry var;
> >+	IpcVarEntry var = IPC_VAR_ENTRY__INIT;
> >+	int ret = -1;
> >+
> >+	var.n_sem_ctls	= 4;
> >+	var.sem_ctls	= xmalloc(pb_repeated_size(&var, sem_ctls));
> 
> Off-topic: is "pb_repeated_size" our helper or generic one?

It's our helper from protobuf.h

> >  static void show_ipc_sem_entries(int fd)
> >  {
> >+	IpcSemEntry *entry;
> >+	u16 *values;
> >+
> >  	pr_msg("\nSemaphores sets:\n");
> >  	while (1) {
> >  		int size;
> >-		struct ipc_sem_entry entry;
> >-		u16 *values;
> >
> >-		if (read_img_eof(fd, &entry) <= 0)
> >-			return;
> >-		pr_msg_ipc_sem_entry(&entry);
> >-		size = sizeof(u16) * entry.nsems;
> >+		entry = NULL;
> 
> Looks like this assignment is redundant.

yeah, a nit ;)

> >  		if (read_img_buf(fd, values, round_up(size, sizeof(u64))) <= 0)
> 
> Probably, "value" should be a repeated field in sem protobuf
> description? Binary, for example.

Could be, but Stas, initially I wanted to make as close map to original
image.h structures as possible and make code change less intrusive.
So if we wanna change it, lets do it on top then.

> 
> >-			return;
> >-		pr_msg_ipc_sem_array(entry.nsems, values);
> >+			break;
> >+		pr_msg_ipc_sem_array(entry->nsems, values);
> >+
> >+		ipc_sem_entry__free_unpacked(entry, NULL);
> >+		xfree(values);
> >  	}
> >+
> >+	xfree(values);
> >+	if (entry)
> >+		ipc_sem_entry__free_unpacked(entry, NULL);
> 
> Is this an optimization? I.e. allocating this entry only once and
> releasing it at the end?

Nope, it's allocated at every cycle and freed at the end of cycle.
The latest

	if (entry)
		ipc_sem_entry__free_unpacked(entry, NULL);

handles the case where xmalloc for array is failed but entry
already read and allocated.

> >+		if (lseek(fd, round_up(shm->size, sizeof(u32)), SEEK_CUR) == (off_t) -1)
> >+			ret = -1;
> >+
> >+		ipc_shm_entry__free_unpacked(shm, NULL);
> 
> 
> Maybe we should use one way of managing PB unpacked entries? I.e.
> either alloc and free only once per image or once per message.

Well, it could be good optimization I think, but lets stick with
simple first ;)

> >
> >-		if (msg.msize > MSGMAX) {
> >+		if (msg->msize > MSGMAX) {
> >+			ret = -1;
> >  			pr_err("Unsupported message size: %d (MAX: %d)\n",
> >-						msg.msize, MSGMAX);
> >-			return ret;
> >+						msg->msize, MSGMAX);
> >+			goto out;
> 
> "out" label is redundant. Break is enough. This gives us minus 1 line.

yeah, thanks!

	Cyrill