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

[Stanislav Kinsbursky]

18.07.2012 14:30, 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.
>

So, please do. Excessive usage of headers make me scary and crying...

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

Cool. Looks like Pavel didn't take it yet.

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

Ok. Just wanna to keep code solid.

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

I see. Looks bad from my POW, but I have nothing else against.

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


-- 
Best regards,
Stanislav Kinsbursky