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

From: Dmitrii Banshchikov <me@ubique.spb.ru>
To: Song Liu <song@kernel.org>
Cc: bpf <bpf@vger.kernel.org>, Alexei Starovoitov <ast@kernel.org>,
	"David S . Miller" <davem@davemloft.net>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Andrii Nakryiko <andrii@kernel.org>,
	Martin KaFai Lau <kafai@fb.com>, Song Liu <songliubraving@fb.com>,
	Yonghong Song <yhs@fb.com>,
	John Fastabend <john.fastabend@gmail.com>,
	KP Singh <kpsingh@kernel.org>,
	Networking <netdev@vger.kernel.org>, Andrey Ignatov <rdna@fb.com>
Subject: Re: [PATCH bpf-next 02/11] bpfilter: Add logging facility
Date: Thu, 20 May 2021 11:08:07 +0400	[thread overview]
Message-ID: <20210520070807.cpmloff4urdsifuy@amnesia> (raw)
In-Reply-To: <CAPhsuW4osuNOagPRwUB30tk3V=ECANktt9jzb+NK1mqOamouSQ@mail.gmail.com>

On Wed, May 19, 2021 at 10:32:25AM -0700, Song Liu wrote:
> 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.?

I found that I need one level for development - to trace what
goes rignt and wrong. At the same time as those messages go to
dmesg this level is too verbose to be used under normal
circumstances. That is why another level is introduced. And the
last one exists to verify invariants or error condintions from
which there is no right way to recover and they result in
bpfilter termination.

Probably we may have just two levels - DEBUG and NOTICE and some
analogue of BUG_ON/WARN_ON/runtime assert that results in a
message on NOTICE level and program termination if the checked
condition is false.

I don't think that we will need more levels - until we decide to
utilize syslog facility. Even in that case I don't know how to
differntiate between e.g. NOTICE and INFO messages.

> 
> >
> > 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.

Sure.

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

-- 

Dmitrii Banshchikov