Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stdin driver with file opener refactors #1605

Merged
merged 54 commits into from Aug 11, 2017

Conversation

bazsi
Copy link
Collaborator

@bazsi bazsi commented Jul 15, 2017

This branch implements the stdin() driver together with a pretty large refactoring step to make this as
simple as possible. This replaces the original #1487 PR, which I've closed a few minutes ago.

The opening of a file, construction of LogTransport and LogProtoServer/Client instances has
been delegated to a new FileOpener interface, making the rest of the code basically independenent of
how a specific file was opened.

This eliminates a large number of conditionals, scattered around the affile code and creates "use-case" specific modules for each specialization of the file driver (named pipes, regular files, /proc/kmsg, /dev/kmsg, and the new one stdin).

The stdin driver does not depend on /dev/stdin as a valid filename anymore, it simply reuses fd 0. The stdin driver also causes syslog-ng to exit once it hits EOF.

There are still rough edges here and there, but I think I could go on with this branch forever :) I believe it is now in a good form so that it is ready for integration.

The patches were split to small baby-steps (albeit a few patches are still huge) to make review easier.

Finally, this is what I've used to test the new stdin() driver:

@version: 3.10

log { 
	source { stdin(); };
	destination { file("/dev/stdout"); };
};

The stdin() driver supports all parameters that file sources do, e.g. multi-line support, flags(no-parse) all work.

@bazsi bazsi force-pushed the stdin-driver-with-file-opener-refactors branch from 988ab52 to 232f5e4 Compare July 15, 2017 23:11
@kira-syslogng
Copy link
Contributor

Build SUCCESS, the tests were executed on test branch: master and test suite: functions

1 similar comment
@kira-syslogng
Copy link
Contributor

Build SUCCESS, the tests were executed on test branch: master and test suite: functions

bazsi added 23 commits August 8, 2017 13:01
Earlier only a note was saying that get_fd() is a layering violation. The
reason is that the presence of this function exposes that all LogProto
instances are based on a UNIX fd. This is always true as of now, but
still not nice. It would be a lot better to replace log_proto_server_get_fd()
with APIs that abstract away fds.

Signed-off-by: Balazs Scheidler <balazs.scheidler@balabit.com>
This constant contains the number of bytes allocated
in LogProtoServerOptionsStorage, so that derived classes may add new fields
in their own Options structure.

Actually, "32" the original value is smaller than LogProtoServerOptions,
e.g. no extra bytes were allocated.

Signed-off-by: Balazs Scheidler <balazs.scheidler@balabit.com>
In order to make this available for derived classes. For now it's just
an alias for the one in buffered server.

Signed-off-by: Balazs Scheidler <balazs.scheidler@balabit.com>
Signed-off-by: Balazs Scheidler <balazs.scheidler@balabit.com>
I find it is a bit easier to read this way.

Signed-off-by: Balazs Scheidler <balazs.scheidler@balabit.com>
log_reader_reopen() was once also doing log_reader_set_options(), which at
that point may have meant changing the stats_source/stats_level values.

Then, reopen and set_options were divorced, however the
log_source_init()/deinit() calls remained, which are not really good at
multiple fronts (at first it reinialized part of the LogReader, but not the
whole).

Anyway, because of this, a stats counter might be registered "none" as the
first log_reader_reopen() is called before set_options, thus stats_source is
not yet set.

To fix this issue, get rid off the superfluous init/deinit calls, which are
not needed.

I also tried to decouple reopen/set_options from the ordering issues, but I
miserably failed, and I didn't want to go there with 50+ patches already in
my series.

Would be great if we could remove this ordering dependency at one point in
the future.

Signed-off-by: Balazs Scheidler <balazs.scheidler@balabit.com>
Signed-off-by: Balazs Scheidler <balazs.scheidler@balabit.com>
Signed-off-by: Balazs Scheidler <balazs.scheidler@balabit.com>
Signed-off-by: Balazs Scheidler <balazs.scheidler@balabit.com>
Instead of keeping these static, make them available for derived classes.

Signed-off-by: Balazs Scheidler <balazs.scheidler@balabit.com>
Signed-off-by: Balazs Scheidler <balazs.scheidler@balabit.com>
Signed-off-by: Balazs Scheidler <balazs.scheidler@balabit.com>
Signed-off-by: Balazs Scheidler <balazs.scheidler@balabit.com>
As the next step creating the FileOpener interface, encapsulate
FilePermOptions into FileOpenerOptions.

Signed-off-by: Balazs Scheidler <balazs.scheidler@balabit.com>
… Options

FileReaderOptions incrporates a number of Options structure, yet it
was missing the init/deinit/defaults interface, so all callers
were explicitly poking into the struct, intializing members. This
is very error prone and this patch introduces the
file_reader_options_init/deinit functions, so that everything
underneath is hidden.

It removes code duplication between affile-source and wildcard-source
and makes the code a lot easier to follow.

Signed-off-by: Balazs Scheidler <balazs.scheidler@balabit.com>
Instead of poking this value right into the FileReader instance,
pass it as a parameter.

Also, this patch renames the longish file_reader_options in FileReader
to merely "options" making the code easier to read.

Signed-off-by: Balazs Scheidler <balazs.scheidler@balabit.com>
Signed-off-by: Balazs Scheidler <balazs.scheidler@balabit.com>
Signed-off-by: Balazs Scheidler <balazs.scheidler@balabit.com>
Signed-off-by: Balazs Scheidler <balazs.scheidler@balabit.com>
Signed-off-by: Balazs Scheidler <balazs.scheidler@balabit.com>
Instead of passing using the "is_pipe" value, expect the caller
to specify the stats source explicitly.

Signed-off-by: Balazs Scheidler <balazs.scheidler@balabit.com>
…earlier

This patch moves the creation of FileOpener instances into the respective
constructors, so that higher level constructors can pass in
different FileOpener instances.

Signed-off-by: Balazs Scheidler <balazs.scheidler@balabit.com>
bazsi added 21 commits August 8, 2017 13:02
This makes the file reader a lot simpler as it does not need /proc/kmsg
and /dev/kmsg specific code.

This patch also gets rid off the separate logproto-linux-proc-kmsg-reader
module.

Signed-off-by: Balazs Scheidler <balazs.scheidler@balabit.com>
Signed-off-by: Balazs Scheidler <balazs.scheidler@balabit.com>
Signed-off-by: Balazs Scheidler <balazs.scheidler@balabit.com>
Signed-off-by: Balazs Scheidler <balazs.scheidler@balabit.com>
Instead of getting this via the log_writer_set_options() call,
get them via LogWriterOptions.

Signed-off-by: Balazs Scheidler <balazs.scheidler@balabit.com>
Instead of getting this via the log_reader_set_options() call,
get them via LogReaderOptions.

Signed-off-by: Balazs Scheidler <balazs.scheidler@balabit.com>
…t_options

Signed-off-by: Balazs Scheidler <balazs.scheidler@balabit.com>
Signed-off-by: Balazs Scheidler <balazs.scheidler@balabit.com>
Signed-off-by: Balazs Scheidler <balazs.scheidler@balabit.com>
Signed-off-by: Balazs Scheidler <balazs.scheidler@balabit.com>
This new class consolidates multi-line options into a class, that makes
it easy to embed multi-line support in any LogReader based sources.

Signed-off-by: Balazs Scheidler <balazs.scheidler@balabit.com>
In a similar spirit to LogProtoFileWriter, we introduce a LogProtoFileWriter,
which is basically a set of options and a constructor that
creates the appropriate LogProtoServer instance based on these options.

Somewhat unfortunately FileReader and LogProtoFileReader are pretty similar
names, which could be fixed by changing the FileReader name to something
like FileReaderPipe, or AFFileSourceReader (akin to AFFileDestWriter).

Signed-off-by: Balazs Scheidler <balazs.scheidler@balabit.com>
Instead of putting these into variables here and there, but them where
they belong, the Options structure of our LogProtoServer instance.

Signed-off-by: Balazs Scheidler <balazs.scheidler@balabit.com>
Signed-off-by: Balazs Scheidler <balazs.scheidler@balabit.com>
Signed-off-by: Balazs Scheidler <balazs.scheidler@balabit.com>
Signed-off-by: Balazs Scheidler <balazs.scheidler@balabit.com>
Signed-off-by: Balazs Scheidler <balazs.scheidler@balabit.com>
Signed-off-by: Balazs Scheidler <balazs.scheidler@balabit.com>
Signed-off-by: Balazs Scheidler <balazs.scheidler@balabit.com>
Signed-off-by: Balazs Scheidler <balazs.scheidler@balabit.com>
@bazsi bazsi force-pushed the stdin-driver-with-file-opener-refactors branch from 232f5e4 to 0ef0aa2 Compare August 8, 2017 11:04
@kira-syslogng
Copy link
Contributor

Build SUCCESS, the tests were executed on test branch: master and test suite: functions

@faxm0dem
Copy link
Contributor

faxm0dem commented Aug 9, 2017

I'm a bit surprised by this supersession, although I haven't looked at the details.
Does this mean all the CLI functionality is dropped?

@lbudai lbudai merged commit 2568eba into master Aug 11, 2017
@bazsi bazsi deleted the stdin-driver-with-file-opener-refactors branch August 15, 2017 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants