Re: [PATCH RFC] Send checkpoint and restart debug info to a log file (v2)

[Oren Laadan <orenl-RdfvBDnrOixBDgjK7y7TUQ@public.gmane.org>]

Serge E. Hallyn wrote:
> Until now, debug data was sent to syslog.  That's not really
> right.
> 
> This patch sends ckpt_debug output to a logfile provided by
> the caller.  If no logfile is provided, then no debug output
> is needed.  So the user can pass in -1 for the logfd to say
> so.

I suggest to keep an option to write to the console/syslog too.
Otherwise we may lose important debug info if a nasty crash
occurs ...

> 
> Note that this does not address the potential (inevitable?)
> lockup of writing out a debug msg while checkpointing the
> debug file.  In practice so far it seems to work rather well.
> (with quite a bit of testing)

I believe the only concern here is that our debug code should
not write any debug info while the respective inode is locked.

> 
> This also means that we have to be more careful than we have
> been about not writing out sensitive data.
> 
> This is pure RFC, not meant to be pretty.
> 
> The split into ckpt_debug and ckpt_err (see changelog) suggests
> that we should rearrange (and be more consistent about) how and
> when we print out debug info.  Left as an exercise for later.

I hate homework...

Besides, I think we need to address it now. This patch is pretty
intrusive and will be painful when I fold to ckpt-v19. I do not
want to go through the process twice.

It's definitely time to come up with guidelines to when/where/how
to add debugging output, and when/where/how to add error output.

In the 'how' section, besides avoiding leaking sensitive data, I'd
like to come up with a canonical format that will be easy to read
and to parse automatically (improve over ckpt_write_err).

Hmm... ckpt_write_err() should change too -- be written to the log
instead.

> 
> Changelog:
> 	Oct 21: split ckpt_debug into ckpt_debug and ckpt_err.
> 		Git rid of the split by memory debug info etc.

The split is useful to control the amount of log.

> 		Since userspace actively asks for debug info, I
> 		also made it not depend on CONFIG_CHECKPOINT_DEBUG.
> 		We may want to put it back again to limit kernel
> 		size, but for now it's a distraction, and I'm not
> 		convinced it makes sense

Then add CONFIG_CHECKPOINT_LOGGING ?
(automatically implied by CONFIG_CHECKPOINT_DEBUG)

> 
> Signed-off-by: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> ---
>  arch/s390/kernel/compat_wrapper.S |    2 +
>  arch/x86/mm/checkpoint.c          |   11 ++---
>  checkpoint/checkpoint.c           |   22 ++++----
>  checkpoint/files.c                |   29 +++++------
>  checkpoint/memory.c               |   43 ++++++++-------
>  checkpoint/namespace.c            |    3 -
>  checkpoint/objhash.c              |   33 +++++++------
>  checkpoint/process.c              |   85 +++++++++++++++----------------
>  checkpoint/restart.c              |  101 +++++++++++++++++--------------------
>  checkpoint/signal.c               |    5 +--
>  checkpoint/sys.c                  |   94 ++++++++++++++++++++++------------
>  drivers/char/tty_io.c             |   22 ++++----
>  include/linux/checkpoint.h        |   85 ++++++++++++++++---------------
>  include/linux/checkpoint_types.h  |    5 +-
>  include/linux/syscalls.h          |    5 +-
>  ipc/checkpoint.c                  |   13 ++---
>  ipc/checkpoint_msg.c              |   13 ++---
>  ipc/checkpoint_sem.c              |   13 ++---
>  ipc/checkpoint_shm.c              |   15 ++---
>  kernel/cred.c                     |    2 +-
>  lib/Kconfig.debug                 |    4 +-
>  mm/filemap.c                      |    2 +-
>  mm/shmem.c                        |    2 +-
>  net/checkpoint.c                  |   46 ++++++++--------
>  net/ipv4/checkpoint.c             |   26 ++++-----
>  net/unix/checkpoint.c             |   65 +++++++++++++-----------
>  26 files changed, 380 insertions(+), 366 deletions(-)

Uhhh... when this matures from RFC to patch-for-inclusion, make
sure to split it nicely for me ... please.

Oren.