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

[Serge E. Hallyn]

Quoting Oren Laadan (orenl at librato.com):
> 
> 
> 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.

Ok, so then current suggested format looks like:

[current->pid]:[ctx->errno]:[ctx->total]

Though is ctx->errno helpful even in restart?  Or should we assume we'll
figure that out through the msg in restore_notify_error()?

Maybe do:
[current->pid]=[task_pid_vnr(current)]:[ctx->total]

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

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