| Message ID | 20210517225308.720677-3-me@ubique.spb.ru (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | bpfilter | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for bpf-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 3 maintainers not CCed: yuehaibing@huawei.com masahiroy@kernel.org kuba@kernel.org |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? WARNING: line length of 100 exceeds 80 columns WARNING: line length of 89 exceeds 80 columns WARNING: line length of 91 exceeds 80 columns |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
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 >
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 > >
> On May 20, 2021, at 12:08 AM, Dmitrii Banshchikov <me@ubique.spb.ru> wrote: > > 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. /dev/kmsg supports specifying priority of the message. Like: echo '<4> This message have priority of 4' > /dev/kmsg Therefore, with proper priority settings, we can have more levels safely. Does this make sense? Thanks, Song [...]
On Thu, May 20, 2021 at 04:35:45PM +0000, Song Liu wrote: > > > > On May 20, 2021, at 12:08 AM, Dmitrii Banshchikov <me@ubique.spb.ru> wrote: > > > > 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. > > /dev/kmsg supports specifying priority of the message. Like: > > echo '<4> This message have priority of 4' > /dev/kmsg > > Therefore, with proper priority settings, we can have more levels safely. > Does this make sense? Yes, it makes. BPFILTER_FATAL should be renamed to BPFILTER_EMERG to match printk() counterpart. All bpfilter log levels should match printk() levels. All bpfilter log messages should include log level. And BPFILTER_DEBUG should be easily turned on/off during compilation to enable tracing/debug. > > Thanks, > Song > > [...] >
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
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. 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 ++++++++++++++++ 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