[CRIU] [PATCH 4/4] log: Introduce log levels

[Kir Kolyshkin]

On 02/17/2012 02:08 PM, Cyrill Gorcunov wrote:
> Signed-off-by: Cyrill Gorcunov<gorcunov at openvz.org>
> ---
>   crtools.c     |    9 ++++++++-
>   include/log.h |   41 +++++++++++++++--------------------------
>   log.c         |   15 ++++++++++++++-
>   3 files changed, 37 insertions(+), 28 deletions(-)
>
> diff --git a/crtools.c b/crtools.c
> index 1305b5a..10db4d5 100644
> --- a/crtools.c
> +++ b/crtools.c
> @@ -291,7 +291,7 @@ int main(int argc, char *argv[])
>   	int action = -1;
>   	int log_inited = 0;
>
> -	static const char short_opts[] = "dsf:p:t:hcD:o:n:";
> +	static const char short_opts[] = "dsf:p:t:hcD:o:n:v:";
>
>   	BUILD_BUG_ON(PAGE_SIZE != PAGE_IMAGE_SIZE);
>
> @@ -344,6 +344,9 @@ int main(int argc, char *argv[])
>   			if (parse_ns_string(optarg,&opts.namespaces_flags))
>   				return -1;
>   			break;
> +		case 'v':
> +			set_loglevel((unsigned int)atoi(optarg));
> +			break;

Can we please make -v argument optional? Like if there's just -v, we 
increase the log level by one
(by two in case of -vv etc), or if you specify an argument it is 
explicit log level.

That way it is
(1) we don't lose any functionality (ie you can still set log level)
(2) simpler to enable debug (just -v)

>   		case 'h':
>   		default:
>   			goto usage;
> @@ -410,6 +413,10 @@ usage:
>
>   	printk("\nAdditional common parameters:\n");
>   	printk("  -D dir         save checkpoint files in specified directory\n");
> +	printk("  -v num         settup logging level\n");
s/settup/set/
> +	printk("                 0 - silent\n");
This is not actually silent, right?

s/silent/silent (only error messages)/

> +	printk("                 1 - informative (default)\n");
> +	printk("                 2 - debug\n");
>   	printk("\n");
>
>   	return -1;
> diff --git a/include/log.h b/include/log.h
> index e6eaaa5..975d2b7 100644
> --- a/include/log.h
> +++ b/include/log.h
> @@ -1,40 +1,29 @@
>   #ifndef LOG_H__
>   #define LOG_H__
>
> -extern void printk(const char *format, ...) __attribute__ ((__format__ (__printf__, 1, 2)));
> -

This stuff was in the previous

>   extern int init_log(const char *name);
>   extern void fini_log(void);
>   extern int get_logfd(void);
>
> -#define pr_info(fmt, ...)	printk(fmt, ##__VA_ARGS__)
> -#define pr_err(fmt, ...)	printk("Error (%s:%d): " fmt, __FILE__, __LINE__, ##__VA_ARGS__)
> -#define pr_panic(fmt, ...)	printk("PANIC (%s:%d): " fmt, __FILE__, __LINE__, ##__VA_ARGS__)
> -#define pr_warning(fmt, ...)	printk("Warning (%s:%d): " fmt, __FILE__, __LINE__, ##__VA_ARGS__)
> +#define LOG_SILENT	(1)
I would rename LOG_SILENT to LOG_ERROR

> +#define LOG_INFO	(2)
> +#define LOG_DEBUG	(3)
> +
> +extern void set_loglevel(unsigned int level);
> +extern void printk_level(unsigned int level, const char *format, ...) __attribute__ ((__format__ (__printf__, 2, 3)));

I am not sure what our coding style is, and I tend to agree that maybe 
80 chars wide is way too restrictive, but let's not make very long 
lines, it's even hard to review.

What about 100 characters per line limit?

> +
> +#define printk(fmt, ...)	printk_level(LOG_SILENT, fmt, ##__VA_ARGS__)

Why printk has a LOG_SILENT level? Shouldn't it be LOG_INFO?

> +#define pr_info(fmt, ...)	printk_level(LOG_INFO,   fmt, ##__VA_ARGS__)
> +#define pr_err(fmt, ...)	printk_level(LOG_SILENT, "Error (%s:%d): " fmt, __FILE__, __LINE__, ##__VA_ARGS__)
> +#define pr_panic(fmt, ...)	printk_level(LOG_SILENT, "Panic (%s:%d): " fmt, __FILE__, __LINE__, ##__VA_ARGS__)
> +#define pr_warning(fmt, ...)	printk_level(LOG_INFO,	 "Warn  (%s:%d): " fmt, __FILE__, __LINE__, ##__VA_ARGS__)
> +#define pr_debug(fmt, ...)	printk_level(LOG_DEBUG,	 "Debug (%s:%d): " fmt, __FILE__, __LINE__, ##__VA_ARGS__)
> +#define pr_perror(fmt, ...)	pr_err(fmt ": %m\n", ##__VA_ARGS__)
>
>   #ifdef CR_DEBUG
> -#define pr_debug(fmt, ...)					\
> -	do {							\
> -		printk("%s:%d:%s: " fmt,			\
> -		       __FILE__, __LINE__,__func__,		\
> -		       ##__VA_ARGS__);				\
> -	} while (0)
> -#define dprintk(fmt, ...)	printk(fmt, ##__VA_ARGS__)
> +#define dprintk(fmt, ...)	printk_level(LOG_DEBUG, fmt, ##__VA_ARGS__)
>   #else
> -#define pr_debug(fmt, ...)
>   #define dprintk(fmt, ...)
>   #endif
>
> -#define die(fmt, ...)						\
> -	do {							\
> -		printk("die (%s:%d): " fmt, __FILE__,		\
> -			__LINE__, ##__VA_ARGS__);		\
> -		exit(1);					\
> -	} while (0)
> -
> -#define pr_perror(fmt, ...)					\
> -	do {							\
> -		pr_err(fmt ": %m\n", ##__VA_ARGS__);		\
> -	} while (0)
> -
>   #endif /* LOG_H__ */
> diff --git a/log.c b/log.c
> index 3776873..433befc 100644
> --- a/log.c
> +++ b/log.c
> @@ -67,10 +67,23 @@ void fini_log(void)
>   	logfd = STDERR_FILENO;
>   }
>
> -void printk(const char *format, ...)
> +static unsigned int loglevel = LOG_INFO;
> +
> +void set_loglevel(unsigned int level)
> +{
> +	if (level>= LOG_DEBUG)
> +		loglevel = LOG_DEBUG;
> +
> +	loglevel = level;
> +}
> +
> +void printk_level(unsigned int level, const char *format, ...)
>   {
>   	va_list params;
>
> +	if (level>  loglevel)
> +		return;
> +
>   	va_start(params, format);
>   	vdprintf(get_logfd(), format, params);
>   	va_end(params);