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

Afsql threaded dest driver #2097

Merged
merged 14 commits into from Oct 31, 2018
Merged

Afsql threaded dest driver #2097

merged 14 commits into from Oct 31, 2018

Conversation

bazsi
Copy link
Collaborator

@bazsi bazsi commented Jun 6, 2018

This branch converts our afsql driver to use the LogThreadedDestDriver framework, instead of its home-grewn implementation (that has originally been the basis of LogThreadedDestDriver).

This should go in after #2315 because without that some of our stats counters would be missing.

@bazsi bazsi changed the base branch from master to logthreadeddst-multi-threading June 6, 2018 21:34
@bazsi bazsi force-pushed the logthreadeddst-multi-threading branch from fbac557 to 46b6159 Compare June 6, 2018 21:35
@bazsi bazsi force-pushed the afsql-threaded-dest-driver branch from 8af60a6 to 4e237e9 Compare June 6, 2018 21:35
@kira-syslogng
Copy link
Contributor

Build FAILURE

@bazsi bazsi force-pushed the logthreadeddst-multi-threading branch from 46b6159 to df239bc Compare June 6, 2018 21:41
@kira-syslogng
Copy link
Contributor

Build FAILURE

1 similar comment
@kira-syslogng
Copy link
Contributor

Build FAILURE

@bazsi bazsi force-pushed the logthreadeddst-multi-threading branch from df239bc to bfcdcd3 Compare June 7, 2018 07:44
@bazsi bazsi force-pushed the afsql-threaded-dest-driver branch from 4e237e9 to cfd79b7 Compare June 7, 2018 07:47
@kira-syslogng
Copy link
Contributor

Build FAILURE

1 similar comment
@kira-syslogng
Copy link
Contributor

Build FAILURE

@bazsi bazsi force-pushed the logthreadeddst-multi-threading branch from 67785c6 to d738cf7 Compare June 7, 2018 16:27
@kira-syslogng
Copy link
Contributor

Build FAILURE

@bazsi bazsi force-pushed the logthreadeddst-multi-threading branch from d738cf7 to 80d3e7b Compare June 19, 2018 07:48
@kira-syslogng
Copy link
Contributor

Build FAILURE

@bazsi bazsi force-pushed the logthreadeddst-multi-threading branch from 80d3e7b to 511c49d Compare June 22, 2018 13:57
@kira-syslogng
Copy link
Contributor

Build FAILURE

@bazsi bazsi force-pushed the logthreadeddst-multi-threading branch from 511c49d to 4aa7ae5 Compare June 29, 2018 16:04
@kira-syslogng
Copy link
Contributor

Build FAILURE

@bazsi bazsi force-pushed the logthreadeddst-multi-threading branch from 4aa7ae5 to 9418c8b Compare July 4, 2018 08:18
@kira-syslogng
Copy link
Contributor

Build FAILURE

@bazsi bazsi force-pushed the logthreadeddst-multi-threading branch from 9418c8b to 1ca1267 Compare July 13, 2018 06:22
@kira-syslogng
Copy link
Contributor

Build FAILURE

@bazsi bazsi force-pushed the logthreadeddst-multi-threading branch from 1ca1267 to 6a1ecdb Compare July 13, 2018 12:31
@kira-syslogng
Copy link
Contributor

Build FAILURE

@bazsi bazsi force-pushed the logthreadeddst-multi-threading branch from 044c18c to 53f2424 Compare July 16, 2018 10:25
@kira-syslogng
Copy link
Contributor

Build FAILURE

@bazsi bazsi force-pushed the logthreadeddst-multi-threading branch from 53f2424 to a790427 Compare July 18, 2018 14:14
@kira-syslogng
Copy link
Contributor

Build FAILURE

@kira-syslogng
Copy link
Contributor

Build FAILURE

bazsi and others added 5 commits October 15, 2018 17:26
Signed-off-by: Balazs Scheidler <balazs.scheidler@balabit.com>
Signed-off-by: Balazs Scheidler <balazs.scheidler@oneidentity.com>
Since logthrdestdrv provides generic support for flush-timeout(), it
is trivial to enable.

Signed-off-by: Balazs Scheidler <balazs.scheidler@oneidentity.com>
Signed-off-by: Balazs Scheidler <balazs.scheidler@oneidentity.com>
@kira-syslogng
Copy link
Contributor

Build FAILURE

@bazsi
Copy link
Collaborator Author

bazsi commented Oct 15, 2018

@kira-syslogng retest this please;

@kira-syslogng
Copy link
Contributor

Build FAILURE

@bazsi
Copy link
Collaborator Author

bazsi commented Oct 15, 2018

@kira-syslogng retest this please

@kira-syslogng
Copy link
Contributor

Build SUCCESS

@MrAnno MrAnno self-requested a review October 17, 2018 08:52
@Ruha0309i
Copy link

👍👍👍👍

@furiel
Copy link
Collaborator

furiel commented Oct 21, 2018

@bazsi I checked with valgrind, and I found an invalid read:

Config:

@version: 3.18

options {
  time-reopen(1);
  stats-level(2);
};

destination d_sql {
    sql(type(sqlite3)
    database("/run/user/1000/testdb.db")
    table("test")
    columns("datetime", "host", "program", "pid", "message")
    values("{$R_DATE}", "${HOST}", "${PROGRAM}", "${PID}", "${MSGONLY}")
    indexes("datetime", "host", "program", "pid", "message")
    flush-lines(2)
    flags(explicit-commits)
    );
};

log {
    source { stdin(flags(no-parse)); };
    destination(d_sql);
    flags(flow-control);
};

The invalid read happens right after syslog-ng starts, without messages.

==8475== Invalid read of size 1
==8475==    at 0x94BC3FB: ??? (in /usr/lib/x86_64-linux-gnu/dbd/libdbdsqlite3.so)
==8475==    by 0x799A85E: dbi_conn_connect (in /usr/lib/x86_64-linux-gnu/libdbi.so.1.1.0)
==8475==    by 0x7789B85: afsql_dd_connect (afsql.c:691)
==8475==    by 0x4E873D1: _compat_connect (logthrdestdrv.c:744)
==8475==    by 0x4E85A79: log_threaded_dest_worker_connect (logthrdestdrv.h:153)
==8475==    by 0x4E85FE9: _connect (logthrdestdrv.c:177)
==8475==    by 0x4E86967: _perform_work (logthrdestdrv.c:440)
==8475==    by 0x603C056: ??? (in /usr/lib/x86_64-linux-gnu/libivykis.so.0.5.4)
==8475==    by 0x603F38D: iv_main (in /usr/lib/x86_64-linux-gnu/libivykis.so.0.5.4)
==8475==    by 0x4E87051: _worker_thread (logthrdestdrv.c:640)
==8475==    by 0x4E8E1FE: _worker_thread_func (mainloop-worker.c:339)
==8475==    by 0x51A5C54: ??? (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4800.2)
==8475==  Address 0x7339e4f is 1 bytes before a block of size 26 alloc'd
==8475==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==8475==    by 0x94BC394: ??? (in /usr/lib/x86_64-linux-gnu/dbd/libdbdsqlite3.so)
==8475==    by 0x799A85E: dbi_conn_connect (in /usr/lib/x86_64-linux-gnu/libdbi.so.1.1.0)
==8475==    by 0x7789B85: afsql_dd_connect (afsql.c:691)
==8475==    by 0x4E873D1: _compat_connect (logthrdestdrv.c:744)
==8475==    by 0x4E85A79: log_threaded_dest_worker_connect (logthrdestdrv.h:153)
==8475==    by 0x4E85FE9: _connect (logthrdestdrv.c:177)
==8475==    by 0x4E86967: _perform_work (logthrdestdrv.c:440)
==8475==    by 0x603C056: ??? (in /usr/lib/x86_64-linux-gnu/libivykis.so.0.5.4)
==8475==    by 0x603F38D: iv_main (in /usr/lib/x86_64-linux-gnu/libivykis.so.0.5.4)
==8475==    by 0x4E87051: _worker_thread (logthrdestdrv.c:640)
==8475==    by 0x4E8E1FE: _worker_thread_func (mainloop-worker.c:339)
==8475== 

@bazsi
Copy link
Collaborator Author

bazsi commented Oct 24, 2018

This error is in dbd_sqlite3 and is caused by the fact that we set sqlite3_dbdir to an empty string, which that code does not handle properly.

I am thinking about a possible workaround, however this is clearly not the result of this refactor, but has been in the code for quite some time now.

Signed-off-by: Balazs Scheidler <balazs.scheidler@oneidentity.com>
@bazsi
Copy link
Collaborator Author

bazsi commented Oct 24, 2018

I have added a workaround on top of the existing branch.

@kira-syslogng
Copy link
Contributor

Build SUCCESS

Signed-off-by: Balazs Scheidler <balazs.scheidler@oneidentity.com>
@kira-syslogng
Copy link
Contributor

Build SUCCESS

@furiel
Copy link
Collaborator

furiel commented Oct 25, 2018

I found another leak, but as the previous one, it is not caused by the refactor. _is_table_present receives a db_res parameter, that is only freed in the true branch. As a result: we leak a leak a dbi_result once, when the table is created. We can handle this independently from this patch. Approve from my side.

@pzoleex
Copy link
Collaborator

pzoleex commented Oct 30, 2018

@kira-syslogng retest this please

@kira-syslogng
Copy link
Contributor

Build SUCCESS

@lbudai lbudai merged commit 16eac5f into master Oct 31, 2018
@MrAnno MrAnno removed their request for review October 31, 2018 14:36
@MrAnno MrAnno deleted the afsql-threaded-dest-driver branch November 11, 2018 21:18
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

8 participants