[CRIU] [PATCH 01/12] pstree: Bind CoreEntry to pstree and fill it with registers early

[Pavel Emelyanov]

On 03/01/2013 10:58 PM, Cyrill Gorcunov wrote:
> On Fri, Mar 01, 2013 at 10:44:27PM +0400, Pavel Emelyanov wrote:
>>> -	if (ctl)
>>> -		regs = ctl->regs_orig;
>>> -	else {
>>> -		if (ptrace(PTRACE_GETREGS, pid, NULL, &regs)) {
>>> -			pr_err("Can't obtain GP registers for %d\n", pid);
>>> -			goto err;
>>> -		}
>>> +	if (ptrace(PTRACE_GETREGS, pid, NULL, &regs)) {
>>> +		pr_err("Can't obtain GP registers for %d\n", pid);
>>> +		goto err;
>>>  	}
>>
>> So, do we still have regs duplication?
> 
> We simply _can't_ live without regs duplication for thread leader.
> parasite_ctl must have own copy in cpu _native_ format, while
> CoreEntry'ies carry registers converted to protobuf format.
> 
> I though about to use CoreEntry instead but this implies adding
> a new routines which would convert pb registers to native format,
> which eventually only complicates the code. That's why I left it
> in this way.

OK

>>> +static int collect_regs_seized(struct pstree_item *item)
>>> +{
>>> +	unsigned int i;
>>> +	int ret;
>>> +
>>> +	if (pstree_alloc_cores(item))
>>> +		return -1;
>>> +
>>> +	for (i = 0; i < item->nr_threads; i++) {
>>> +		pid_t pid = item->threads[i].real;
>>> +		ret = get_task_regs(pid, item->core[i]);
>>> +		if (ret) {
>>> +			pr_err("Can't obtain regs for thread %d\n", pid);
>>> +			return -1;
>>> +		}
>>> +
>>> +		if (item->threads[i].real == item->pid.real)
>>> +			item->this_core = item->core[i];
>>
>> We've already done this in pstree_alloc_cores()
> 
> Indeed, this is harmless leftover, thanks!
> 
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>
>>> +int pstree_alloc_cores(struct pstree_item *item)
>>> +{
>>> +	unsigned int i;
>>> +
>>> +	item->core = xzalloc(sizeof(*item->core) * item->nr_threads);
>>> +	if (!item->core)
>>> +		return -1;
>>> +
>>> +	for (i = 0; i < item->nr_threads; i++) {
>>> +		int is_leader = (item->threads[i].real == item->pid.real);
>>> +
>>> +		item->core[i] = core_entry_alloc(item->state != TASK_DEAD, is_leader);
>>> +		if (!item->core[i])
>>> +			goto err;
>>> +
>>> +		if (is_leader)
>>> +			item->this_core = item->core[i];
>>
>> The version in collect_regs_seized() without is_leader looks nicer.
> 
> I wanted to distinguish pstree functions with anything else, ie we
> have pstree_alloc_cores/pstree_free_cores pair which doesn't care
> about the data present in CoreEntry but only serves allocation/frees
> of CoreEntry, and collect_regs_seized which fills core entries with
> registers.
> 
> In first versions of this patch collect_regs_seized didn't allocate
> Cores but you don't like it as far as i remember, that's why I moved
> pstree_alloc_cores into collect_regs_seized.

You didn't get my point.

The 

	if (foo == bar)
		doit();

code looks much better than

	int should_i_doit = (foo == bar);

	if (should_i_doit)
		doit();

> 	Cyrill
> .
>