Re: [PATCH bpf-next 02/11] bpfilter: Add logging facility

[Song Liu <song@kernel.org>]

On Tue, May 18, 2021 at 11:05 PM Dmitrii Banshchikov <me@ubique.spb.ru> wrote:
>
> There are three logging levels for messages: FATAL, NOTICE and DEBUG.
> When a message is logged with FATAL level it results in bpfilter
> usermode helper termination.

Could you please explain why we choose to have 3 levels? Will we need
more levels,
like WARNING, ERROR, etc.?

>
> Introduce struct context to avoid use of global objects and store there
> the logging parameters: log level and log sink.
>
> Signed-off-by: Dmitrii Banshchikov <me@ubique.spb.ru>
> ---
>  net/bpfilter/Makefile  |  2 +-
>  net/bpfilter/bflog.c   | 29 +++++++++++++++++++++++++++++
>  net/bpfilter/bflog.h   | 24 ++++++++++++++++++++++++
>  net/bpfilter/context.h | 16 ++++++++++++++++

Maybe combine bflog.h and context.h into one file? And bflog() can
probably fit in
that file too.

Thanks,
Song

>  4 files changed, 70 insertions(+), 1 deletion(-)
>  create mode 100644 net/bpfilter/bflog.c
>  create mode 100644 net/bpfilter/bflog.h
>  create mode 100644 net/bpfilter/context.h
>
> diff --git a/net/bpfilter/Makefile b/net/bpfilter/Makefile
> index cdac82b8c53a..874d5ef6237d 100644
> --- a/net/bpfilter/Makefile
> +++ b/net/bpfilter/Makefile
> @@ -4,7 +4,7 @@
>  #
>
>  userprogs := bpfilter_umh
> -bpfilter_umh-objs := main.o
> +bpfilter_umh-objs := main.o bflog.o
>  userccflags += -I $(srctree)/tools/include/ -I $(srctree)/tools/include/uapi
>
>  ifeq ($(CONFIG_BPFILTER_UMH), y)
> diff --git a/net/bpfilter/bflog.c b/net/bpfilter/bflog.c
> new file mode 100644
> index 000000000000..2752e39060e4
> --- /dev/null
> +++ b/net/bpfilter/bflog.c
> @@ -0,0 +1,29 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2021 Telegram FZ-LLC
> + */
> +
> +#define _GNU_SOURCE
> +
> +#include "bflog.h"
> +
> +#include <stdarg.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +
> +#include "context.h"
> +
> +void bflog(struct context *ctx, int level, const char *fmt, ...)
> +{
> +       if (ctx->log_file &&
> +           (level == BFLOG_LEVEL_FATAL || (level & ctx->log_level))) {
> +               va_list va;
> +
> +               va_start(va, fmt);
> +               vfprintf(ctx->log_file, fmt, va);
> +               va_end(va);
> +       }
> +
> +       if (level == BFLOG_LEVEL_FATAL)
> +               exit(EXIT_FAILURE);
> +}
> diff --git a/net/bpfilter/bflog.h b/net/bpfilter/bflog.h
> new file mode 100644
> index 000000000000..4ed12791cfa1
> --- /dev/null
> +++ b/net/bpfilter/bflog.h
> @@ -0,0 +1,24 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2021 Telegram FZ-LLC
> + */
> +
> +#ifndef NET_BPFILTER_BFLOG_H
> +#define NET_BPFILTER_BFLOG_H
> +
> +struct context;
> +
> +#define BFLOG_IMPL(ctx, level, fmt, ...) bflog(ctx, level, "bpfilter: " fmt, ##__VA_ARGS__)
> +
> +#define BFLOG_LEVEL_FATAL (0)
> +#define BFLOG_LEVEL_NOTICE (1)
> +#define BFLOG_LEVEL_DEBUG (2)
> +
> +#define BFLOG_FATAL(ctx, fmt, ...)                                                                 \
> +       BFLOG_IMPL(ctx, BFLOG_LEVEL_FATAL, "fatal error: " fmt, ##__VA_ARGS__)
> +#define BFLOG_NOTICE(ctx, fmt, ...) BFLOG_IMPL(ctx, BFLOG_LEVEL_NOTICE, fmt, ##__VA_ARGS__)
> +#define BFLOG_DEBUG(ctx, fmt, ...) BFLOG_IMPL(ctx, BFLOG_LEVEL_DEBUG, fmt, ##__VA_ARGS__)
> +
> +void bflog(struct context *ctx, int level, const char *fmt, ...);
> +
> +#endif // NET_BPFILTER_BFLOG_H
> diff --git a/net/bpfilter/context.h b/net/bpfilter/context.h
> new file mode 100644
> index 000000000000..e85c97c3d010
> --- /dev/null
> +++ b/net/bpfilter/context.h
> @@ -0,0 +1,16 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2021 Telegram FZ-LLC
> + */
> +
> +#ifndef NET_BPFILTER_CONTEXT_H
> +#define NET_BPFILTER_CONTEXT_H
> +
> +#include <stdio.h>
> +
> +struct context {
> +       FILE *log_file;
> +       int log_level;
> +};
> +
> +#endif // NET_BPFILTER_CONTEXT_H
> --
> 2.25.1
>