[Devel] Re: [RFC PATCH 11/17] define function to print error messages to user log

[Oren Laadan]

Serge E. Hallyn wrote:
> Quoting Oren Laadan (orenl at librato.com):
>>
>> Serge E. Hallyn wrote:
>>> Quoting Oren Laadan (orenl at librato.com):
>>>> Serge E. Hallyn wrote:
>>>>> Quoting Matt Helsley (matthltc at us.ibm.com):
>>>>>>> @@ -401,6 +409,9 @@ char *ckpt_generate_fmt(struct ckpt_ctx *ctx, char *fmt)
>>>>>>>  		case 'E':
>>>>>>>  			len += sprintf(format+len, "[%s]", "err %d");
>>>>>>>  			break;
>>>>>>> +		case 'C': /* count of bytes read/written to checkpoint image */
>>>>>>> +			len += sprintf(format+len, "[%s]", "pos %d");
>>>>>>> +			break;
>>>>>> Instead we could always output ckpt->total and then we wouldn't need %(C). I
>>>>>> suspect it's such a useful piece of information that it'll be repeated
>>>>>> in many/all format strings eventually.
>>>>> Yes, likewise %(T).  If that's what we want to do.
>>>> I agree. For the cases when there is not task, can put "none"
>>>>
>>>>> Should we discuss here what we want an entry to look like?  For both
>>>>> ckpt_write_err (to the checkpoint image) and ckpt_error()?
>>>>>
>>>> Yes please !
>>> Actually %T isn't the current task, right, so it shouldn't always be prepended?
>>> It actually is only meaningful during checkpoint_task(), collect_objs(), and
>>> __tree_count_tasks?
>>>
>>> Ok, so how about:
>>>
>>> 	1. ckpt_write_err() always also calls ckpt_error() (which in turn calls
>>> 		ckpt_debug).  Avoid duplication which exists in several places
>>> 		right now.
>>> 	2. We always prepend:
>>>
>>> 		[current->pid]:[ctx->root_pid]:[ctx->active_pid]:[ctx->errno][ctx->total]
>>>
>>> 	The %(X) expansions if specified come whereever they are in the fmt
>>> 	string (which is what's happening now with my patchset).
>> So somewhere should set ctx->errno during a checkpoint.
>>
>> I suppose active_pid is for restart, but it's redundant isn't it ?
>> (it's always active_pid) - is it the different between top-level pid-ns
>> and "current" pid-ns ?
> 
> No, I figured it would be meaningful for instance in places like
> wait_task_active().

Perhaps then leave it out of the default printing, and have the
specific debug messages there write it explicitly.

> 
>> Instead of writing root_pid repeatedly, why not write sometime at the
>> beginning some "global" info about the checkpoint/restart ?  (e.g.
>> the root_pid ...)
> 
> Well it is written out (for restart) at the end, so I suppose I should
> switch restore_debug_free() to using ckpt_error() instead of ckpt_debug().

Yes, that will be helpful to reduce the noise :)

Oren.

_______________________________________________
Containers mailing list
Containers at lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers