[PATCH 0/3] WIP: Convert syslog-ng to use upstream ivykis
A long long time ago, upstream and BalaBit's version of ivykis started to diverge, which - at the time - was necessary, but the intent always was to eventually get things upstream and merged, to allow syslog-ng to use upstream ivykis, as-is. With the help of Lennert Buytenhek (ivykis upstream) and Balazs Scheidler, I present you the first work in progress patch set against syslog-ng that enables it to work with an unmodified ivykis! The patch set is not 100% complete yet however, I'm only submitting it for review and possible testing. I will probably port it to 3.3 too (it applies almost cleanly, there's only one trivial conflict in lib/Makefile.am) and build the Debian packages using upstream ivykis to get wider testing. What is missing, is the old lib/ivykis/configure.gnu script, which we had there to force a static build of ivykis. This will be replaced by a modification to autogen.sh, that will automatically create the configure.gnu scripts in the various submodules, so we can point the submodule links to upstream repos instead of our own forks. This is not included in this set yet, and one will need to copy that file over from the BalaBit fork, or things won't work too well. Another thing is that the first patch stops libsyslog-ng-crypto from being directly linked to ivykis. This is not an issue when ivykis is statically compiled into libsyslog-ng, since all those symbols will be exported to other modules, including libsyslog-ng-crypto. However, it might become a problem when using a dynamically linked ivykis. I haven't tested that case yet. The reason I remove the link is that if ivykis is statically compiled in to both shared objects, the constructors will run twice, as dlopen()ing anything linked against libsyslog-ng-crypto will trigger them again. This leads to an abort() inside ivykis. This can be done on the ivykis side too: separate the constructors from the init functions, and make the constructors no-op if they ran already: this allows the init functions to continue aborting if called more than once, but allows the constructors to run twice. I have to discuss this with upstream, and see if we need this at all, or if my workaround of removing the linkage from libsyslog-ng-crypto is enough and without side effects. These were the easy issues, which I could solve right now if I had some coffee left. But alas, there is another issue! With the first two patches applied, we'd have a syslog-ng that compiles fine against upstream ivykis, and even works. Except it doesn't: whenever we try to set an io handler for an fd, we'd end up in an abort, because for some reason, we already had a handler set. I do not understand it quite yet why this happens - the old BalaBit fork of ivykis was similarly picky and abort-happy and they did not trigger. As a workaround, I created the third patch, because I wanted to have something that shows signs of working. The third patch is *WRONG*, but it's an acceptable workaround until I have time to debug the problem further. What it does, is that it *always* clears the io handlers before registering new ones. This results in marginally more work being done each time we need to set a handler, but not by much, and as far as I saw, it has no side effects. But since the part of ivykis that aborts before the third patch, did not change between BalaBit's fork and upstream, I'm certain that the second patch introduces a bug that wasn't there before. Nevertheless! The good news still is, that we're *very* close to being able to build with upstream ivykis! A few issues to fix, and then comes the next part: going over all the changes that were made to the BalaBit fork of ivykis, and send everything upstream that has not been sent yet. That will be another funny adventure, but at least, by that time, I will understand what the hell I am doing =) -- |8]
There is no need to link libsyslog-ng-crypto to ivykis, because the ivykis symbols will either be available via libsyslog-ng, or through the system installed ivykis. Signed-off-by: Gergely Nagy <algernon@balabit.hu> --- lib/Makefile.am | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Makefile.am b/lib/Makefile.am index e68c9ae..13b2472 100644 --- a/lib/Makefile.am +++ b/lib/Makefile.am @@ -13,7 +13,7 @@ libsyslog_ng_la_LDFLAGS = -no-undefined -release @VERSION@ module_LTLIBRARIES = libsyslog-ng-crypto.la libsyslog_ng_crypto_la_CFLAGS = @UUID_CFLAGS@ -libsyslog_ng_crypto_la_LIBADD = @CORE_DEPS_LIBS@ @OPENSSL_LIBS@ @UUID_LIBS@ libsyslog-ng.la +libsyslog_ng_crypto_la_LIBADD = @SYSLOGNG_DEPS_LIBS@ @OPENSSL_LIBS@ @UUID_LIBS@ libsyslog-ng.la libsyslog_ng_crypto_la_LDFLAGS = -no-undefined -avoid-version # this is intentionally formatted so conflicts are less likely to arise. one name in every line. -- 1.7.10
This patch converts syslog-ng to use upstream ivykis, unmodified. Apart from the boring struct and type renaming stuff, there is one major change: iv_fd_pollable() is gone, we have iv_fd_register_try(). As such, the logic in log_reader_start_watches() and log_writer_start_watches() was updated to use the upstream API instead. Many thanks to Lennert Buytenhek <buytenh@wantstofly.org> and Balazs Scheidler <bazsi@balabit.hu> for their invaluable help. Signed-off-by: Gergely Nagy <algernon@balabit.hu> --- .gitmodules | 2 +- lib/ivykis | 2 +- lib/logmsg.c | 2 +- lib/logmsg.h | 2 +- lib/logqueue-fifo.c | 52 +++++++++++++++++++++++++-------------------------- lib/logreader.c | 31 ++++++++++++++++++------------ lib/logwriter.c | 15 ++++++++------- lib/mainloop.c | 36 +++++++++++++++++------------------ lib/mainloop.h | 6 +++--- 9 files changed, 78 insertions(+), 70 deletions(-) diff --git a/.gitmodules b/.gitmodules index 6254dc1..68956f3 100644 --- a/.gitmodules +++ b/.gitmodules @@ -3,7 +3,7 @@ url = git://git.balabit.hu/bazsi/libmongo-client.git [submodule "lib/ivykis"] path = lib/ivykis - url = git://git.balabit.hu/bazsi/ivykis.git + url = git://github.com/buytenh/ivykis.git [submodule "lib/eventlog"] path = lib/eventlog url = git://git.balabit.hu/bazsi/eventlog-1.0.git diff --git a/lib/ivykis b/lib/ivykis index 73c0c27..905fa75 160000 --- a/lib/ivykis +++ b/lib/ivykis @@ -1 +1 @@ -Subproject commit 73c0c27a19755f700b268081a5a59acdd04b421e +Subproject commit 905fa75c813f0058d383100fd4819a39bc10f45b diff --git a/lib/logmsg.c b/lib/logmsg.c index 31d9c4c..e912c73 100644 --- a/lib/logmsg.c +++ b/lib/logmsg.c @@ -416,7 +416,7 @@ log_msg_is_handle_match(NVHandle handle) static void log_msg_init_queue_node(LogMessage *msg, LogMessageQueueNode *node, const LogPathOptions *path_options) { - INIT_LIST_HEAD(&node->list); + INIT_IV_LIST_HEAD(&node->list); node->ack_needed = path_options->ack_needed; node->msg = log_msg_ref(msg); log_msg_write_protect(msg); diff --git a/lib/logmsg.h b/lib/logmsg.h index 4f0e93f..d322141 100644 --- a/lib/logmsg.h +++ b/lib/logmsg.h @@ -113,7 +113,7 @@ enum typedef struct _LogMessageQueueNode { - struct list_head list; + struct iv_list_head list; LogMessage *msg; gboolean ack_needed:1, embedded:1; } LogMessageQueueNode; diff --git a/lib/logqueue-fifo.c b/lib/logqueue-fifo.c index 9c27a2b..f7a2386 100644 --- a/lib/logqueue-fifo.c +++ b/lib/logqueue-fifo.c @@ -75,18 +75,18 @@ typedef struct _LogQueueFifo LogQueue super; /* scalable qoverflow implementation */ - struct list_head qoverflow_output; - struct list_head qoverflow_wait; + struct iv_list_head qoverflow_output; + struct iv_list_head qoverflow_wait; gint qoverflow_wait_len; gint qoverflow_output_len; gint qoverflow_size; /* in number of elements */ - struct list_head qbacklog; /* entries that were sent but not acked yet */ + struct iv_list_head qbacklog; /* entries that were sent but not acked yet */ gint qbacklog_len; struct { - struct list_head items; + struct iv_list_head items; MainLoopIOWorkerFinishCallback cb; guint16 len; guint16 finish_cb_registered; @@ -150,10 +150,10 @@ log_queue_fifo_move_input_unlocked(LogQueueFifo *self, gint thread_id) for (i = 0; i < n; i++) { - LogMessageQueueNode *node = list_entry(self->qoverflow_input[thread_id].items.next, LogMessageQueueNode, list); + LogMessageQueueNode *node = iv_list_entry(self->qoverflow_input[thread_id].items.next, LogMessageQueueNode, list); LogMessage *msg = node->msg; - list_del(&node->list); + iv_list_del(&node->list); self->qoverflow_input[thread_id].len--; path_options.ack_needed = node->ack_needed; stats_counter_inc(self->super.dropped_messages); @@ -167,7 +167,7 @@ log_queue_fifo_move_input_unlocked(LogQueueFifo *self, gint thread_id) NULL); } stats_counter_add(self->super.stored_messages, self->qoverflow_input[thread_id].len); - list_splice_tail_init(&self->qoverflow_input[thread_id].items, &self->qoverflow_wait); + iv_list_splice_tail_init(&self->qoverflow_input[thread_id].items, &self->qoverflow_wait); self->qoverflow_wait_len += self->qoverflow_input[thread_id].len; self->qoverflow_input[thread_id].len = 0; } @@ -239,7 +239,7 @@ log_queue_fifo_push_tail(LogQueue *s, LogMessage *msg, const LogPathOptions *pat } node = log_msg_alloc_queue_node(msg, path_options); - list_add_tail(&node->list, &self->qoverflow_input[thread_id].items); + iv_list_add_tail(&node->list, &self->qoverflow_input[thread_id].items); self->qoverflow_input[thread_id].len++; log_msg_unref(msg); return; @@ -256,7 +256,7 @@ log_queue_fifo_push_tail(LogQueue *s, LogMessage *msg, const LogPathOptions *pat { node = log_msg_alloc_queue_node(msg, path_options); - list_add_tail(&node->list, &self->qoverflow_wait); + iv_list_add_tail(&node->list, &self->qoverflow_wait); self->qoverflow_wait_len++; log_queue_push_notify(&self->super); @@ -297,7 +297,7 @@ log_queue_fifo_push_head(LogQueue *s, LogMessage *msg, const LogPathOptions *pat log_queue_assert_output_thread(s); node = log_msg_alloc_dynamic_queue_node(msg, path_options); - list_add(&node->list, &self->qoverflow_output); + iv_list_add(&node->list, &self->qoverflow_output); self->qoverflow_output_len++; stats_counter_inc(self->super.stored_messages); @@ -323,7 +323,7 @@ log_queue_fifo_pop_head(LogQueue *s, LogMessage **msg, LogPathOptions *path_opti { /* slow path, output queue is empty, get some elements from the wait queue */ g_static_mutex_lock(&self->super.lock); - list_splice_tail_init(&self->qoverflow_wait, &self->qoverflow_output); + iv_list_splice_tail_init(&self->qoverflow_wait, &self->qoverflow_output); self->qoverflow_output_len = self->qoverflow_wait_len; self->qoverflow_wait_len = 0; g_static_mutex_unlock(&self->super.lock); @@ -331,19 +331,19 @@ log_queue_fifo_pop_head(LogQueue *s, LogMessage **msg, LogPathOptions *path_opti if (self->qoverflow_output_len > 0) { - node = list_entry(self->qoverflow_output.next, LogMessageQueueNode, list); + node = iv_list_entry(self->qoverflow_output.next, LogMessageQueueNode, list); *msg = node->msg; path_options->ack_needed = node->ack_needed; self->qoverflow_output_len--; if (!push_to_backlog) { - list_del(&node->list); + iv_list_del(&node->list); log_msg_free_queue_node(node); } else { - list_del_init(&node->list); + iv_list_del_init(&node->list); } } else @@ -363,7 +363,7 @@ log_queue_fifo_pop_head(LogQueue *s, LogMessage **msg, LogPathOptions *path_opti if (push_to_backlog) { log_msg_ref(*msg); - list_add_tail(&node->list, &self->qbacklog); + iv_list_add_tail(&node->list, &self->qbacklog); self->qbacklog_len++; } if (!ignore_throttle && self->super.throttle_buckets > 0) @@ -391,11 +391,11 @@ log_queue_fifo_ack_backlog(LogQueue *s, gint n) { LogMessageQueueNode *node; - node = list_entry(self->qbacklog.next, LogMessageQueueNode, list); + node = iv_list_entry(self->qbacklog.next, LogMessageQueueNode, list); msg = node->msg; path_options.ack_needed = node->ack_needed; - list_del(&node->list); + iv_list_del(&node->list); log_msg_free_queue_node(node); self->qbacklog_len--; @@ -422,23 +422,23 @@ log_queue_fifo_rewind_backlog(LogQueue *s) log_queue_assert_output_thread(s); - list_splice_tail_init(&self->qbacklog, &self->qoverflow_output); + iv_list_splice_tail_init(&self->qbacklog, &self->qoverflow_output); self->qoverflow_output_len += self->qbacklog_len; stats_counter_add(self->super.stored_messages, self->qbacklog_len); self->qbacklog_len = 0; } static void -log_queue_fifo_free_queue(struct list_head *q) +log_queue_fifo_free_queue(struct iv_list_head *q) { - while (!list_empty(q)) + while (!iv_list_empty(q)) { LogMessageQueueNode *node; LogPathOptions path_options = LOG_PATH_OPTIONS_INIT; LogMessage *msg; - node = list_entry(q->next, LogMessageQueueNode, list); - list_del(&node->list); + node = iv_list_entry(q->next, LogMessageQueueNode, list); + iv_list_del(&node->list); path_options.ack_needed = node->ack_needed; msg = node->msg; @@ -484,14 +484,14 @@ log_queue_fifo_new(gint qoverflow_size, const gchar *persist_name) for (i = 0; i < log_queue_max_threads; i++) { - INIT_LIST_HEAD(&self->qoverflow_input[i].items); + INIT_IV_LIST_HEAD(&self->qoverflow_input[i].items); main_loop_io_worker_finish_callback_init(&self->qoverflow_input[i].cb); self->qoverflow_input[i].cb.user_data = self; self->qoverflow_input[i].cb.func = log_queue_fifo_move_input; } - INIT_LIST_HEAD(&self->qoverflow_wait); - INIT_LIST_HEAD(&self->qoverflow_output); - INIT_LIST_HEAD(&self->qbacklog); + INIT_IV_LIST_HEAD(&self->qoverflow_wait); + INIT_IV_LIST_HEAD(&self->qoverflow_output); + INIT_IV_LIST_HEAD(&self->qbacklog); self->qoverflow_size = qoverflow_size; return &self->super; diff --git a/lib/logreader.c b/lib/logreader.c index 562edd9..5a35e06 100644 --- a/lib/logreader.c +++ b/lib/logreader.c @@ -329,9 +329,6 @@ log_reader_start_watches(LogReader *self) log_proto_prepare(self->proto, &fd, &cond); - if (self->pollable_state < 0 && fd >= 0) - self->pollable_state = iv_fd_pollable(fd); - if (self->options->follow_freq > 0) { /* follow freq specified (only the file source does that, go into timed polling */ @@ -345,18 +342,28 @@ log_reader_start_watches(LogReader *self) NULL); return FALSE; } - else if (self->pollable_state > 0) + else { /* we have an FD, it is possible to poll it, register it */ self->fd_watch.fd = fd; - iv_fd_register(&self->fd_watch); - } - else - { - msg_error("Unable to determine how to monitor this fd, follow_freq() not set and it is not possible to poll it with the current ivykis polling method, try changing IV_EXCLUDE_POLL_METHOD environment variable", - evt_tag_int("fd", fd), - NULL); - return FALSE; + if (self->pollable_state < 0) + { + if (iv_fd_register_try(&self->fd_watch) == 0) + self->pollable_state = 1; + else + self->pollable_state = 0; + } + else if (self->pollable_state > 0) + { + iv_fd_register(&self->fd_watch); + } + else + { + msg_error("Unable to determine how to monitor this fd, follow_freq() not set and it is not possible to poll it with the current ivykis polling method, try changing IV_EXCLUDE_POLL_METHOD environment variable", + evt_tag_int("fd", fd), + NULL); + return FALSE; + } } log_reader_update_watches(self); diff --git a/lib/logwriter.c b/lib/logwriter.c index 91a2b27..6d0c28f 100644 --- a/lib/logwriter.c +++ b/lib/logwriter.c @@ -418,19 +418,20 @@ log_writer_start_watches(LogWriter *self) { log_proto_prepare(self->proto, &fd, &cond); + self->fd_watch.fd = fd; + if (self->pollable_state < 0) { + /* auto-detect if the fd can be polled using ivykis */ if (is_file_regular(fd)) self->pollable_state = 0; + else if (iv_fd_register_try(&self->fd_watch) == 0) + self->pollable_state = 1; else - self->pollable_state = iv_fd_pollable(fd); - } - - if (self->pollable_state) - { - self->fd_watch.fd = fd; - iv_fd_register(&self->fd_watch); + self->pollable_state = 0; } + else if (self->pollable_state > 0) + iv_fd_register(&self->fd_watch); log_writer_update_watches(self); self->watches_running = TRUE; diff --git a/lib/mainloop.c b/lib/mainloop.c index c9fc8b0..35a6836 100644 --- a/lib/mainloop.c +++ b/lib/mainloop.c @@ -116,7 +116,7 @@ static GlobalConfig *current_configuration; typedef struct _MainLoopTaskCallSite MainLoopTaskCallSite; struct _MainLoopTaskCallSite { - struct list_head list; + struct iv_list_head list; MainLoopTaskFunc func; gpointer user_data; gpointer result; @@ -135,7 +135,7 @@ TLS_BLOCK_START TLS_BLOCK_END; static GStaticMutex main_task_lock = G_STATIC_MUTEX_INIT; -static struct list_head main_task_queue = LIST_HEAD_INIT(main_task_queue); +static struct iv_list_head main_task_queue = IV_LIST_HEAD_INIT(main_task_queue); static struct iv_event main_task_posted; GThread *main_thread_handle; @@ -166,14 +166,14 @@ main_loop_call(MainLoopTaskFunc func, gpointer user_data, gboolean wait) } /* call_info.lock is no longer needed, since we're the only ones using call_info now */ - INIT_LIST_HEAD(&call_info.list); + INIT_IV_LIST_HEAD(&call_info.list); call_info.pending = TRUE; call_info.func = func; call_info.user_data = user_data; call_info.wait = wait; if (!call_info.cond) call_info.cond = g_cond_new(); - list_add(&call_info.list, &main_task_queue); + iv_list_add(&call_info.list, &main_task_queue); iv_event_post(&main_task_posted); if (wait) { @@ -188,13 +188,13 @@ static void main_loop_call_handler(gpointer user_data) { g_static_mutex_lock(&main_task_lock); - while (!list_empty(&main_task_queue)) + while (!iv_list_empty(&main_task_queue)) { MainLoopTaskCallSite *site; gpointer result; - site = list_entry(main_task_queue.next, MainLoopTaskCallSite, list); - list_del_init(&site->list); + site = iv_list_entry(main_task_queue.next, MainLoopTaskCallSite, list); + iv_list_del_init(&site->list); g_static_mutex_unlock(&main_task_lock); result = site->func(site->user_data); @@ -362,21 +362,21 @@ main_loop_io_worker_job_submit(MainLoopIOWorkerJob *self) static void main_loop_io_worker_job_start(MainLoopIOWorkerJob *self) { - struct list_head *lh, *lh2; + struct iv_list_head *lh, *lh2; g_assert(main_loop_current_job == NULL); main_loop_current_job = self; self->work(self->user_data); - list_for_each_safe(lh, lh2, &self->finish_callbacks) + iv_list_for_each_safe(lh, lh2, &self->finish_callbacks) { - MainLoopIOWorkerFinishCallback *cb = list_entry(lh, MainLoopIOWorkerFinishCallback, list); + MainLoopIOWorkerFinishCallback *cb = iv_list_entry(lh, MainLoopIOWorkerFinishCallback, list); cb->func(cb->user_data); - list_del_init(&cb->list); + iv_list_del_init(&cb->list); } - g_assert(list_empty(&self->finish_callbacks)); + g_assert(iv_list_empty(&self->finish_callbacks)); main_loop_current_job = NULL; } @@ -432,7 +432,7 @@ main_loop_io_worker_register_finish_callback(MainLoopIOWorkerFinishCallback *cb) { g_assert(main_loop_current_job != NULL); - list_add(&cb->list, &main_loop_current_job->finish_callbacks); + iv_list_add(&cb->list, &main_loop_current_job->finish_callbacks); } void @@ -442,7 +442,7 @@ main_loop_io_worker_job_init(MainLoopIOWorkerJob *self) self->work_item.cookie = self; self->work_item.work = (void (*)(void *)) main_loop_io_worker_job_start; self->work_item.completion = (void (*)(void *)) main_loop_io_worker_job_complete; - INIT_LIST_HEAD(&self->finish_callbacks); + INIT_IV_LIST_HEAD(&self->finish_callbacks); } static void @@ -705,26 +705,26 @@ main_loop_run(void) IV_SIGNAL_INIT(&sighup_poll); sighup_poll.signum = SIGHUP; - sighup_poll.exclusive = 1; + sighup_poll.flags = IV_SIGNAL_FLAG_EXCLUSIVE; sighup_poll.cookie = NULL; sighup_poll.handler = sig_hup_handler; iv_signal_register(&sighup_poll); IV_SIGNAL_INIT(&sigchild_poll); sigchild_poll.signum = SIGCHLD; - sigchild_poll.exclusive = 1; + sigchild_poll.flags = IV_SIGNAL_FLAG_EXCLUSIVE; sigchild_poll.handler = sig_child_handler; iv_signal_register(&sigchild_poll); IV_SIGNAL_INIT(&sigterm_poll); sigterm_poll.signum = SIGTERM; - sigterm_poll.exclusive = 1; + sigterm_poll.flags = IV_SIGNAL_FLAG_EXCLUSIVE; sigterm_poll.handler = sig_term_handler; iv_signal_register(&sigterm_poll); IV_SIGNAL_INIT(&sigint_poll); sigint_poll.signum = SIGINT; - sigint_poll.exclusive = 1; + sigint_poll.flags = IV_SIGNAL_FLAG_EXCLUSIVE; sigint_poll.handler = sig_term_handler; iv_signal_register(&sigint_poll); diff --git a/lib/mainloop.h b/lib/mainloop.h index c432718..cb02d8a 100644 --- a/lib/mainloop.h +++ b/lib/mainloop.h @@ -36,7 +36,7 @@ typedef gpointer (*MainLoopTaskFunc)(gpointer user_data); typedef struct _MainLoopIOWorkerFinishCallback { - struct list_head list; + struct iv_list_head list; MainLoopTaskFunc func; gpointer user_data; } MainLoopIOWorkerFinishCallback; @@ -44,7 +44,7 @@ typedef struct _MainLoopIOWorkerFinishCallback static inline void main_loop_io_worker_finish_callback_init(MainLoopIOWorkerFinishCallback *self) { - INIT_LIST_HEAD(&self->list); + INIT_IV_LIST_HEAD(&self->list); } typedef struct _MainLoopIOWorkerJob @@ -56,7 +56,7 @@ typedef struct _MainLoopIOWorkerJob struct iv_work_item work_item; /* function to be called back when the current job is finished. */ - struct list_head finish_callbacks; + struct iv_list_head finish_callbacks; } MainLoopIOWorkerJob; static inline gboolean -- 1.7.10
The iv_fd_set_handler_in()/iv_fd_set_handler_out() functions are picky, and will abort() if we want to set a handler for an FD that already has handlers registered. To avoid that, whenever we want to set a handler, clear the old one first. This is perhaps not the best approach, as the above two functions were similarly picky before migrating to upstream's ivykis, yet, they did not abort() away. So there's probably some other underlying problem lurking somewhere still, but for the time being, this will do. At least, syslog-ng compiles AND works properly this way. Signed-off-by: Gergely Nagy <algernon@balabit.hu> --- lib/control.c | 12 ++++-------- lib/logreader.c | 11 ++++------- lib/logwriter.c | 8 ++++++-- 3 files changed, 14 insertions(+), 17 deletions(-) diff --git a/lib/control.c b/lib/control.c index 52ec095..715beb4 100644 --- a/lib/control.c +++ b/lib/control.c @@ -278,16 +278,12 @@ control_connection_stop_watches(ControlConnection *self) static void control_connection_update_watches(ControlConnection *self) { + iv_fd_set_handler_in(&self->control_io, NULL); + iv_fd_set_handler_out(&self->control_io, NULL); if (self->output_buffer->len > self->pos) - { - iv_fd_set_handler_out(&self->control_io, control_connection_io_output); - iv_fd_set_handler_in(&self->control_io, NULL); - } + iv_fd_set_handler_out(&self->control_io, control_connection_io_output); else - { - iv_fd_set_handler_out(&self->control_io, NULL); - iv_fd_set_handler_in(&self->control_io, control_connection_io_input); - } + iv_fd_set_handler_in(&self->control_io, control_connection_io_input); } ControlConnection * diff --git a/lib/logreader.c b/lib/logreader.c index 5a35e06..889d074 100644 --- a/lib/logreader.c +++ b/lib/logreader.c @@ -451,21 +451,18 @@ log_reader_update_watches(LogReader *self) * files cannot be polled using epoll, as it causes an I/O error * (thus abort in ivykis). */ + iv_fd_set_handler_in(&self->fd_watch, NULL); + iv_fd_set_handler_out(&self->fd_watch, NULL); + iv_fd_set_handler_err(&self->fd_watch, NULL); + if (cond & G_IO_IN) iv_fd_set_handler_in(&self->fd_watch, log_reader_io_process_input); - else - iv_fd_set_handler_in(&self->fd_watch, NULL); if (cond & G_IO_OUT) iv_fd_set_handler_out(&self->fd_watch, log_reader_io_process_input); - else - iv_fd_set_handler_out(&self->fd_watch, NULL); if (cond & (G_IO_IN + G_IO_OUT)) iv_fd_set_handler_err(&self->fd_watch, log_reader_io_process_input); - else - iv_fd_set_handler_err(&self->fd_watch, NULL); - } else { diff --git a/lib/logwriter.c b/lib/logwriter.c index 6d0c28f..5d2daa9 100644 --- a/lib/logwriter.c +++ b/lib/logwriter.c @@ -251,6 +251,9 @@ log_writer_update_fd_callbacks(LogWriter *self, GIOCondition cond) main_loop_assert_main_thread(); if (self->pollable_state > 0) { + /* By default, we're not interested in the input, and before + setting a handler, we have to clear the old one first too. */ + iv_fd_set_handler_in(&self->fd_watch, NULL); if (self->flags & LW_DETECT_EOF && (cond & G_IO_IN) == 0 && (cond & G_IO_OUT)) { /* if output is enabled, and we're in DETECT_EOF mode, and input is @@ -272,11 +275,12 @@ log_writer_update_fd_callbacks(LogWriter *self, GIOCondition cond) /* otherwise we're not interested in input */ iv_fd_set_handler_in(&self->fd_watch, NULL); } + + iv_fd_set_handler_out(&self->fd_watch, NULL); if (cond & G_IO_OUT) iv_fd_set_handler_out(&self->fd_watch, log_writer_io_flush_output); - else - iv_fd_set_handler_out(&self->fd_watch, NULL); + iv_fd_set_handler_err(&self->fd_watch, NULL); iv_fd_set_handler_err(&self->fd_watch, log_writer_io_error); } else -- 1.7.10
On Mon, May 07, 2012 at 11:42:33PM +0200, Gergely Nagy wrote:
The iv_fd_set_handler_in()/iv_fd_set_handler_out() functions are picky, and will abort() if we want to set a handler for an FD that already has handlers registered. To avoid that, whenever we want to set a handler, clear the old one first.
Actually, that shouldn't happen. I'm looking at the code, and that looks like a bug -- probably started happening when we started doing delayed flushing of fds to the kernel. I think it's safe to just delete those abort() calls from iv_fd_set_handler_{in,out,err}().
Lennert Buytenhek <buytenh@wantstofly.org> writes:
On Mon, May 07, 2012 at 11:42:33PM +0200, Gergely Nagy wrote:
The iv_fd_set_handler_in()/iv_fd_set_handler_out() functions are picky, and will abort() if we want to set a handler for an FD that already has handlers registered. To avoid that, whenever we want to set a handler, clear the old one first.
Actually, that shouldn't happen.
I'm looking at the code, and that looks like a bug -- probably started happening when we started doing delayed flushing of fds to the kernel.
I think it's safe to just delete those abort() calls from iv_fd_set_handler_{in,out,err}().
Oh, right. There *was* a change in ivykis that could affect the behaviour of iv_fd_set_handler_{in,out,err}()! Thanks for the reminder. I'll submit a patch in ~10 hours or so, after testing with syslog-ng, unless you beat me to it. -- |8]
On Tue, May 08, 2012 at 12:11:54AM +0200, Gergely Nagy wrote:
The iv_fd_set_handler_in()/iv_fd_set_handler_out() functions are picky, and will abort() if we want to set a handler for an FD that already has handlers registered. To avoid that, whenever we want to set a handler, clear the old one first.
Actually, that shouldn't happen.
I'm looking at the code, and that looks like a bug -- probably started happening when we started doing delayed flushing of fds to the kernel.
I think it's safe to just delete those abort() calls from iv_fd_set_handler_{in,out,err}().
Oh, right. There *was* a change in ivykis that could affect the behaviour of iv_fd_set_handler_{in,out,err}()!
Thanks for the reminder. I'll submit a patch in ~10 hours or so, after testing with syslog-ng, unless you beat me to it.
Go for it. You found the issue, you get the credit. :-)
On Mon, May 07, 2012 at 11:42:30PM +0200, Gergely Nagy wrote:
A long long time ago, upstream and BalaBit's version of ivykis started to diverge, which - at the time - was necessary, but the intent always was to eventually get things upstream and merged, to allow syslog-ng to use upstream ivykis, as-is.
With the help of Lennert Buytenhek (ivykis upstream) and Balazs Scheidler, I present you the first work in progress patch set against syslog-ng that enables it to work with an unmodified ivykis!
(Whoops, I didn't see this email until now.) Yay! Thanks for doing this work.
Another thing is that the first patch stops libsyslog-ng-crypto from being directly linked to ivykis. This is not an issue when ivykis is statically compiled into libsyslog-ng, since all those symbols will be exported to other modules, including libsyslog-ng-crypto. However, it might become a problem when using a dynamically linked ivykis. I haven't tested that case yet.
The reason I remove the link is that if ivykis is statically compiled in to both shared objects, the constructors will run twice, as dlopen()ing anything linked against libsyslog-ng-crypto will trigger them again. This leads to an abort() inside ivykis.
This can be done on the ivykis side too: separate the constructors from the init functions, and make the constructors no-op if they ran already: this allows the init functions to continue aborting if called more than once, but allows the constructors to run twice. I have to discuss this with upstream, and see if we need this at all, or if my workaround of removing the linkage from libsyslog-ng-crypto is enough and without side effects.
If the constructors run twice but bss variables are still shared, then yeah, we should do something like what you said.
But alas, there is another issue!
With the first two patches applied, we'd have a syslog-ng that compiles fine against upstream ivykis, and even works. Except it doesn't: whenever we try to set an io handler for an fd, we'd end up in an abort, because for some reason, we already had a handler set.
I do not understand it quite yet why this happens - the old BalaBit fork of ivykis was similarly picky and abort-happy and they did not trigger.
(I screwed up here -- mea culpa -- but let's discuss this in the other thread.)
Nevertheless! The good news still is, that we're *very* close to being able to build with upstream ivykis! A few issues to fix, and then comes the next part: going over all the changes that were made to the BalaBit fork of ivykis, and send everything upstream that has not been sent yet.
Right, there's several changes still in your version of ivykis that haven't made it into mine (e.g. to have an abstraction for syslog(), which seems a generally good idea to have, and probably there's more bits as well). Do you think syslog-ng is ready for inclusion into e.g. Fedora as-is, or do you want to get those changes merged into upstream ivykis as well first? cheers, Lennert
Lennert Buytenhek <buytenh@wantstofly.org> writes:
On Mon, May 07, 2012 at 11:42:30PM +0200, Gergely Nagy wrote:
A long long time ago, upstream and BalaBit's version of ivykis started to diverge, which - at the time - was necessary, but the intent always was to eventually get things upstream and merged, to allow syslog-ng to use upstream ivykis, as-is.
With the help of Lennert Buytenhek (ivykis upstream) and Balazs Scheidler, I present you the first work in progress patch set against syslog-ng that enables it to work with an unmodified ivykis!
(Whoops, I didn't see this email until now.)
Yay! Thanks for doing this work.
Most of the work has been done by you and Bazsi. I'm just putting the pieces together. O:)
Another thing is that the first patch stops libsyslog-ng-crypto from being directly linked to ivykis. This is not an issue when ivykis is statically compiled into libsyslog-ng, since all those symbols will be exported to other modules, including libsyslog-ng-crypto. However, it might become a problem when using a dynamically linked ivykis. I haven't tested that case yet.
The reason I remove the link is that if ivykis is statically compiled in to both shared objects, the constructors will run twice, as dlopen()ing anything linked against libsyslog-ng-crypto will trigger them again. This leads to an abort() inside ivykis.
This can be done on the ivykis side too: separate the constructors from the init functions, and make the constructors no-op if they ran already: this allows the init functions to continue aborting if called more than once, but allows the constructors to run twice. I have to discuss this with upstream, and see if we need this at all, or if my workaround of removing the linkage from libsyslog-ng-crypto is enough and without side effects.
If the constructors run twice but bss variables are still shared, then yeah, we should do something like what you said.
As far as I see, what happens is that when syslog-ng is started, the ivykis statically linked into libsyslog-ng gets loaded and the constructors run. So far, so good, that's how it is supposed to work! When syslog-ng dlopen()s a module linked against libsyslog-ng-crypto, which also has an ivykis statically linked in, ld.so notices that the symbols are already resolved, so won't load them again, BUT the constructors *are* different, since the constructor can be thought of as a function in the lib that runs whatever we had an __attribute__((constructor))__ on. This means that the init functions will run again, even though they were not redefined, because the newly loaded constructor-collecting function-like thingy does get run regardless. This means that constructors and destructors should be made idempotent. Which might be tricky if we consider that a module can (in theory, syslog-ng doesn't do this AFAIK) be unloaded, which would trigger the destructor, while the main code would still use it. So deinit should only be called from the destructors if there are no users left. This makes things a little bit more complicated, I think. For syslog-ng, it's enough if nothing but libsyslog-ng gets linked to ivykis, so my workaround will do the trick. For other things, though, this may not be enough.
Nevertheless! The good news still is, that we're *very* close to being able to build with upstream ivykis! A few issues to fix, and then comes the next part: going over all the changes that were made to the BalaBit fork of ivykis, and send everything upstream that has not been sent yet.
Right, there's several changes still in your version of ivykis that haven't made it into mine (e.g. to have an abstraction for syslog(), which seems a generally good idea to have, and probably there's more bits as well). Do you think syslog-ng is ready for inclusion into e.g. Fedora as-is, or do you want to get those changes merged into upstream ivykis as well first?
The move to upstream ivykis is something that I wouldn't feel comfortable with doing in 3.3 (at least not yet), and 3.4 is still only alpha, and I wouldn't push it towards Fedora just yet. However, as far as I'm concerned, syslog-ng 3.4 + current ivykis (with the io handlers fixed, patch will be on its way in a bit) is ready to be experimented with, and included in development/experimental distro branches (such as OpenSUSE Factory and Debian experimental, and whatever Fedora has - if anything - that is similar in spirit). The other differences can be resolved later, since 3.4 is still in development. In the long run, when everything was sent your way upstream and the code differences resolved, I might consider merging the patches into syslog-ng 3.3, but I want to be very careful about it. I prefer fixing things instead of accidentally breaking them, and replacing the forked ivykis with upstream is quite an invasive change. -- |8]
On Tue, May 08, 2012 at 10:34:06AM +0200, Gergely Nagy wrote:
Nevertheless! The good news still is, that we're *very* close to being able to build with upstream ivykis! A few issues to fix, and then comes the next part: going over all the changes that were made to the BalaBit fork of ivykis, and send everything upstream that has not been sent yet.
Right, there's several changes still in your version of ivykis that haven't made it into mine (e.g. to have an abstraction for syslog(), which seems a generally good idea to have, and probably there's more bits as well). Do you think syslog-ng is ready for inclusion into e.g. Fedora as-is, or do you want to get those changes merged into upstream ivykis as well first?
The move to upstream ivykis is something that I wouldn't feel comfortable with doing in 3.3 (at least not yet), and 3.4 is still only alpha, and I wouldn't push it towards Fedora just yet.
However, as far as I'm concerned, syslog-ng 3.4 + current ivykis (with the io handlers fixed, patch will be on its way in a bit) is ready to be experimented with, and included in development/experimental distro branches (such as OpenSUSE Factory and Debian experimental, and whatever Fedora has - if anything - that is similar in spirit). The other differences can be resolved later, since 3.4 is still in development.
In the long run, when everything was sent your way upstream and the code differences resolved, I might consider merging the patches into syslog-ng 3.3, but I want to be very careful about it. I prefer fixing things instead of accidentally breaking them, and replacing the forked ivykis with upstream is quite an invasive change.
OK, clear. This seems reasonable.
On Tue, May 08, 2012 at 10:34:06AM +0200, Gergely Nagy wrote:
Another thing is that the first patch stops libsyslog-ng-crypto from being directly linked to ivykis. This is not an issue when ivykis is statically compiled into libsyslog-ng, since all those symbols will be exported to other modules, including libsyslog-ng-crypto. However, it might become a problem when using a dynamically linked ivykis. I haven't tested that case yet.
The reason I remove the link is that if ivykis is statically compiled in to both shared objects, the constructors will run twice, as dlopen()ing anything linked against libsyslog-ng-crypto will trigger them again. This leads to an abort() inside ivykis.
This can be done on the ivykis side too: separate the constructors from the init functions, and make the constructors no-op if they ran already: this allows the init functions to continue aborting if called more than once, but allows the constructors to run twice. I have to discuss this with upstream, and see if we need this at all, or if my workaround of removing the linkage from libsyslog-ng-crypto is enough and without side effects.
If the constructors run twice but bss variables are still shared, then yeah, we should do something like what you said.
As far as I see, what happens is that when syslog-ng is started, the ivykis statically linked into libsyslog-ng gets loaded and the constructors run. So far, so good, that's how it is supposed to work!
When syslog-ng dlopen()s a module linked against libsyslog-ng-crypto, which also has an ivykis statically linked in, ld.so notices that the symbols are already resolved, so won't load them again, BUT the constructors *are* different, since the constructor can be thought of as a function in the lib that runs whatever we had an __attribute__((constructor))__ on.
This means that the init functions will run again, even though they were not redefined, because the newly loaded constructor-collecting function-like thingy does get run regardless.
This means that constructors and destructors should be made idempotent. Which might be tricky if we consider that a module can (in theory, syslog-ng doesn't do this AFAIK) be unloaded, which would trigger the destructor, while the main code would still use it.
So deinit should only be called from the destructors if there are no users left.
This makes things a little bit more complicated, I think. For syslog-ng, it's enough if nothing but libsyslog-ng gets linked to ivykis, so my workaround will do the trick. For other things, though, this may not be enough.
I thought about this for a while, but couldn't really figure out how this should work under the hood. So I tried it out in practice, and I indeed can't seem to make it work in a way that makes sense. I whipped up the following test setup: tst.c statically linking against libtest.a (built from libtest.c), and dynamically loading libplugin.so (built from libplugin.c) which is also statically linked against libtest.a. First of all, I need to build libtest.a with -fPIC, or gcc won't let me link the non-PIC code in libtest.a into libplugin.so (which only makes sense, I guess). Do you need to do that too, or am I testing something different from your setup at this point? Having done that, I indeed see a constructor function in libtest.c being called both before main() and when libplugin.so is dlopen()ed, i.e. it's called twice like what you report above. But then, if I define a static variable in libtest.c, and print its address in that constructor function, I get different addresses for each of the invocations of the constructor. I.e. the libtest.a linked into tst.c is using a different set of static variables than the libtest.a linked into libplugin.so. If the same happens in your setup, I don't see how we can fix this except building libivykis as a .so instead of as a .a -- but if you're not seeing this, I'd love to know what I'm doing wrong... (If I make the global variable in libtest.c non-static, both invocations of the libtest constructor see the same address for the variable. But, most global ivykis variables are static..)
Lennert Buytenhek <buytenh@wantstofly.org> writes:
I thought about this for a while, but couldn't really figure out how this should work under the hood. So I tried it out in practice, and I indeed can't seem to make it work in a way that makes sense.
I whipped up the following test setup: tst.c statically linking against libtest.a (built from libtest.c), and dynamically loading libplugin.so (built from libplugin.c) which is also statically linked against libtest.a.
First of all, I need to build libtest.a with -fPIC, or gcc won't let me link the non-PIC code in libtest.a into libplugin.so (which only makes sense, I guess). Do you need to do that too, or am I testing something different from your setup at this point?
From what I see, we're building everything with -fPIC, both libivykis.a, and libsyslog-ng.so and libsyslog-ng-crypto.so. So it seems to me we're testing similar scenarios.
Having done that, I indeed see a constructor function in libtest.c being called both before main() and when libplugin.so is dlopen()ed, i.e. it's called twice like what you report above.
But then, if I define a static variable in libtest.c, and print its address in that constructor function, I get different addresses for each of the invocations of the constructor. I.e. the libtest.a linked into tst.c is using a different set of static variables than the libtest.a linked into libplugin.so.
If the same happens in your setup, I don't see how we can fix this except building libivykis as a .so instead of as a .a -- but if you're not seeing this, I'd love to know what I'm doing wrong...
I'm seeing something else, I think. I digged a bit further, and found that if I place a break on iv_tls_user_register, I get this when syslog-ng starts up: (gdb) bt #0 iv_tls_user_register (itu=0x7ffff7ddb940) at ../../../../lib/ivykis/lib/iv_tls.c:34 #1 0x00007ffff7ba0e7c in iv_event_tls_init () at ../../../../lib/ivykis/modules/iv_event.c:98 #2 0x00007ffff7deacd0 in ?? () from /lib64/ld-linux-x86-64.so.2 #3 0x00007ffff7deadc7 in ?? () from /lib64/ld-linux-x86-64.so.2 #4 0x00007ffff7dddb2a in ?? () from /lib64/ld-linux-x86-64.so.2 (gdb) p &inited $3 = (int *) 0x7ffff7ddcaf0 (gdb) p inited $4 = 0 Hitting continue a few times, so that every ivykis module registers their tls stuff, I end up with: (gdb) bt #0 iv_tls_user_register (itu=0x7ffff59f9b20) at ../../../../lib/ivykis/lib/iv_tls.c:34 #1 0x00007ffff57f20d0 in iv_event_tls_init () at ../../../../lib/ivykis/modules/iv_event.c:98 #2 0x00007ffff7deacd0 in ?? () from /lib64/ld-linux-x86-64.so.2 #3 0x00007ffff7deadc7 in ?? () from /lib64/ld-linux-x86-64.so.2 #4 0x00007ffff7def0b3 in ?? () from /lib64/ld-linux-x86-64.so.2 #5 0x00007ffff7dea926 in ?? () from /lib64/ld-linux-x86-64.so.2 #6 0x00007ffff7dee89a in ?? () from /lib64/ld-linux-x86-64.so.2 #7 0x00007ffff67c7f66 in dlopen_doit (a=<optimized out>) at dlopen.c:67 #8 0x00007ffff7dea926 in ?? () from /lib64/ld-linux-x86-64.so.2 #9 0x00007ffff67c82ec in _dlerror_run (operate=0x7ffff67c7f00 <dlopen_doit>, args=0x7fffffff89f0) at dlerror.c:164 #10 0x00007ffff67c7ee1 in __dlopen (file=<optimized out>, mode=<optimized out>) at dlopen.c:88 #11 0x00007ffff7714944 in g_module_open () from /usr/lib/x86_64-linux-gnu/libgmodule-2.0.so.0 #12 0x00007ffff7b7c440 in plugin_dlopen_module (module_name=0x63df10 "afsocket", module_path=0x617a50 "/home/algernon/install/syslog-ng/ose-3.4/lib/syslog-ng") at ../../lib/plugin.c:305 #13 0x00007ffff7b7c592 in plugin_load_module (module_name=0x63df10 "afsocket", cfg=0x616930, args=0x617c80) at ../../lib/plugin.c:349 #14 0x00007ffff7b979c9 in pragma_parse (lexer=0x61c530, result=0x7fffffffaff8, arg=0x0) at pragma-grammar.y:397 #15 0x00007ffff7b5300b in cfg_parser_parse (self=0x7ffff7dda8a0, lexer=0x61c530, instance=0x7fffffffaff8, arg=0x0) at ../../lib/cfg-parser.h:83 #16 0x00007ffff7b54bae in cfg_lexer_lex (self=0x61c530, yylval=0x7fffffffd3e0, yylloc=0x7fffffffd3c0) at ../../lib/cfg-lexer.c:733 #17 0x00007ffff7b55b7b in main_lex (yylval=0x7fffffffd3e0, yylloc=0x7fffffffd3c0, lexer=0x61c530) at ../../lib/cfg-parser.c:164 #18 0x00007ffff7b8fda3 in main_parse (lexer=0x61c530, dummy=0x7fffffffd598, arg=0x0) at cfg-grammar.c:3153 #19 0x00007ffff7b51d78 in cfg_parser_parse (self=0x7ffff7dd9a00, lexer=0x61c530, instance=0x7fffffffd598, arg=0x0) at ../../lib/cfg-parser.h:83 #20 0x00007ffff7b529c1 in cfg_run_parser (self=0x616930, lexer=0x61c530, parser=0x7ffff7dd9a00, result=0x7fffffffd598, arg=0x0) at ../../lib/cfg.c:316 #21 0x00007ffff7b52add in cfg_read_config (self=0x616930, fname=0x608050 "etc/mongodb.conf", syntax_only=0, preprocess_into=0x0) at ../../lib/cfg.c:347 #22 0x00007ffff7b75ff3 in main_loop_init () at ../../lib/mainloop.c:677 #23 0x0000000000401994 in main (argc=1, argv=0x7fffffffd708) at ../../syslog-ng/main.c:239 (gdb) p &inited $13 = (int *) 0x7ffff7ddcaf0 (gdb) p inited $14 = 1 inited has the same address, but if you notice, the iv_event_tls_init() that ends up being called is 0x00007ffff7ba0e7c in one case, and 0x00007ffff57f20d0 in the other. On the other hand, iv_tls_user_register() ends up being the same (verified that by disassembling from the breakpoint). I'm not sure this is relevant, though... The iv_event_tls_user structures are different aswell in these two cases, which is expected, since they're static. Truth be told, at this point, I'm leaning towards ignoring the whole problem, and not linking dlopen()-able stuff against ivykis if ivykis is pulled in in another way. In that case the whole problem goes away, and as far as I checked, will work reliably in all cases: - libsyslog-ng.so with libivykis.a statically linked in: the symbols are exported, so dlopen()'d modules are able to use it - OK! - libsyslog-ng.so linked dynamically to libivykis.so: the symbols are available to dlopen()'d modules, so all is well - OK! And that's the only two cases syslog-ng supports, and what anyone would normally support, imo. Statically linking a dlopened module to a library the main lib/binary is linked to will almost always result in compatibility issues, as that would potentially allow for things like syslog-ng linked with ivykis version Y, dlopening a module linked to ivykis version X. All hell would break loose. It's probably better to abort right at the beginning, as a sign that someone somewhere did some bad linkage. -- |8]
On Mon, May 14, 2012 at 12:03:07PM +0200, Gergely Nagy wrote:
Having done that, I indeed see a constructor function in libtest.c being called both before main() and when libplugin.so is dlopen()ed, i.e. it's called twice like what you report above.
But then, if I define a static variable in libtest.c, and print its address in that constructor function, I get different addresses for each of the invocations of the constructor. I.e. the libtest.a linked into tst.c is using a different set of static variables than the libtest.a linked into libplugin.so.
If the same happens in your setup, I don't see how we can fix this except building libivykis as a .so instead of as a .a -- but if you're not seeing this, I'd love to know what I'm doing wrong...
I'm seeing something else, I think. I digged a bit further, and found that if I place a break on iv_tls_user_register, I get this when syslog-ng starts up:
(gdb) bt #0 iv_tls_user_register (itu=0x7ffff7ddb940) at ../../../../lib/ivykis/lib/iv_tls.c:34 #1 0x00007ffff7ba0e7c in iv_event_tls_init () at ../../../../lib/ivykis/modules/iv_event.c:98 #2 0x00007ffff7deacd0 in ?? () from /lib64/ld-linux-x86-64.so.2 #3 0x00007ffff7deadc7 in ?? () from /lib64/ld-linux-x86-64.so.2 #4 0x00007ffff7dddb2a in ?? () from /lib64/ld-linux-x86-64.so.2 (gdb) p &inited $3 = (int *) 0x7ffff7ddcaf0 (gdb) p inited $4 = 0
Hitting continue a few times, so that every ivykis module registers their tls stuff, I end up with:
(gdb) bt #0 iv_tls_user_register (itu=0x7ffff59f9b20) at ../../../../lib/ivykis/lib/iv_tls.c:34 #1 0x00007ffff57f20d0 in iv_event_tls_init () at ../../../../lib/ivykis/modules/iv_event.c:98 #2 0x00007ffff7deacd0 in ?? () from /lib64/ld-linux-x86-64.so.2 #3 0x00007ffff7deadc7 in ?? () from /lib64/ld-linux-x86-64.so.2 #4 0x00007ffff7def0b3 in ?? () from /lib64/ld-linux-x86-64.so.2 #5 0x00007ffff7dea926 in ?? () from /lib64/ld-linux-x86-64.so.2 #6 0x00007ffff7dee89a in ?? () from /lib64/ld-linux-x86-64.so.2 #7 0x00007ffff67c7f66 in dlopen_doit (a=<optimized out>) at dlopen.c:67 #8 0x00007ffff7dea926 in ?? () from /lib64/ld-linux-x86-64.so.2 #9 0x00007ffff67c82ec in _dlerror_run (operate=0x7ffff67c7f00 <dlopen_doit>, args=0x7fffffff89f0) at dlerror.c:164 #10 0x00007ffff67c7ee1 in __dlopen (file=<optimized out>, mode=<optimized out>) at dlopen.c:88 #11 0x00007ffff7714944 in g_module_open () from /usr/lib/x86_64-linux-gnu/libgmodule-2.0.so.0 #12 0x00007ffff7b7c440 in plugin_dlopen_module (module_name=0x63df10 "afsocket", module_path=0x617a50 "/home/algernon/install/syslog-ng/ose-3.4/lib/syslog-ng") at ../../lib/plugin.c:305 #13 0x00007ffff7b7c592 in plugin_load_module (module_name=0x63df10 "afsocket", cfg=0x616930, args=0x617c80) at ../../lib/plugin.c:349 #14 0x00007ffff7b979c9 in pragma_parse (lexer=0x61c530, result=0x7fffffffaff8, arg=0x0) at pragma-grammar.y:397 #15 0x00007ffff7b5300b in cfg_parser_parse (self=0x7ffff7dda8a0, lexer=0x61c530, instance=0x7fffffffaff8, arg=0x0) at ../../lib/cfg-parser.h:83 #16 0x00007ffff7b54bae in cfg_lexer_lex (self=0x61c530, yylval=0x7fffffffd3e0, yylloc=0x7fffffffd3c0) at ../../lib/cfg-lexer.c:733 #17 0x00007ffff7b55b7b in main_lex (yylval=0x7fffffffd3e0, yylloc=0x7fffffffd3c0, lexer=0x61c530) at ../../lib/cfg-parser.c:164 #18 0x00007ffff7b8fda3 in main_parse (lexer=0x61c530, dummy=0x7fffffffd598, arg=0x0) at cfg-grammar.c:3153 #19 0x00007ffff7b51d78 in cfg_parser_parse (self=0x7ffff7dd9a00, lexer=0x61c530, instance=0x7fffffffd598, arg=0x0) at ../../lib/cfg-parser.h:83 #20 0x00007ffff7b529c1 in cfg_run_parser (self=0x616930, lexer=0x61c530, parser=0x7ffff7dd9a00, result=0x7fffffffd598, arg=0x0) at ../../lib/cfg.c:316 #21 0x00007ffff7b52add in cfg_read_config (self=0x616930, fname=0x608050 "etc/mongodb.conf", syntax_only=0, preprocess_into=0x0) at ../../lib/cfg.c:347 #22 0x00007ffff7b75ff3 in main_loop_init () at ../../lib/mainloop.c:677 #23 0x0000000000401994 in main (argc=1, argv=0x7fffffffd708) at ../../syslog-ng/main.c:239 (gdb) p &inited $13 = (int *) 0x7ffff7ddcaf0 (gdb) p inited $14 = 1
inited has the same address,
OK, so that's where our test results diverge -- in my test app, a gdb 'p &mylocalvar' also prints different addresses (for a static global variable).
but if you notice, the iv_event_tls_init() that ends up being called is 0x00007ffff7ba0e7c in one case, and 0x00007ffff57f20d0 in the other. On the other hand, iv_tls_user_register() ends up being the same (verified that by disassembling from the breakpoint). I'm not sure this is relevant, though...
(Probably also because iv_event_tls_init is static and iv_tls_user_register is not..)
The iv_event_tls_user structures are different aswell in these two cases, which is expected, since they're static.
Truth be told, at this point, I'm leaning towards ignoring the whole problem, and not linking dlopen()-able stuff against ivykis if ivykis is pulled in in another way. In that case the whole problem goes away, and as far as I checked, will work reliably in all cases:
- libsyslog-ng.so with libivykis.a statically linked in: the symbols are exported, so dlopen()'d modules are able to use it - OK! - libsyslog-ng.so linked dynamically to libivykis.so: the symbols are available to dlopen()'d modules, so all is well - OK!
That sounds okay to me, then. In case of a static ivykis, I guess it makes the most sense, conceptually, to consider the ivykis code to be part of one of the shared objects, and to provide the ivykis API from that shared object. What you argue for is basically that, so I think we're in agreement.
And that's the only two cases syslog-ng supports, and what anyone would normally support, imo. Statically linking a dlopened module to a library the main lib/binary is linked to will almost always result in compatibility issues, as that would potentially allow for things like syslog-ng linked with ivykis version Y, dlopening a module linked to ivykis version X. All hell would break loose.
It's probably better to abort right at the beginning, as a sign that someone somewhere did some bad linkage.
(Wouldn't symbol versioning help there, too?)
Lennert Buytenhek <buytenh@wantstofly.org> writes:
What you argue for is basically that, so I think we're in agreement.
Yep, we are!
And that's the only two cases syslog-ng supports, and what anyone would normally support, imo. Statically linking a dlopened module to a library the main lib/binary is linked to will almost always result in compatibility issues, as that would potentially allow for things like syslog-ng linked with ivykis version Y, dlopening a module linked to ivykis version X. All hell would break loose.
It's probably better to abort right at the beginning, as a sign that someone somewhere did some bad linkage.
(Wouldn't symbol versioning help there, too?)
Mmm.. Perhaps. It would make it less of an issue. But it's easier to avoid it altogether, and not link ivykis into both the main and the dlopened object, just one of them. -- |8]
Gergely Nagy <algernon@balabit.hu> writes:
A long long time ago, upstream and BalaBit's version of ivykis started to diverge, which - at the time - was necessary, but the intent always was to eventually get things upstream and merged, to allow syslog-ng to use upstream ivykis, as-is.
With the help of Lennert Buytenhek (ivykis upstream) and Balazs Scheidler, I present you the first work in progress patch set against syslog-ng that enables it to work with an unmodified ivykis!
And in an attempt to not spam the list with N+1 revisions of the same patch set, I'd like to direct the interested readers to my feature/3.4/ivykis/upstream[1] branch, which at the moment contains these changes: * "Do not link anything but libsyslog-ng against ivykis" As the summary implies, and discussed before, this makes it so that only libsyslog-ng is linked against ivykis, nothing else is. In addition to yesterday's set, this fixes the afsmtp module too, not only libsyslog-ng-crypto. (These two were the only SOs linked directly against ivykis, apart from libsyslog-ng) * "autogen.sh: Create configure.gnu ourselves in submodules" To make it easier to use upstream git repos as submodules, this changes the autogen.sh script to create configure.gnu itself. Exchanging our forked repos to upstream has never been easier! (This is something I hinted at yesterday, but didn't have time to do then) * "lib/: Use upstream ivykis" This is pretty much the same patch as the bulk of yesterday's set, except that in lib/control.c, there's an extra change that turns an iv_fd_register() into iv_fd_register_try(), as that part of the code relies on the fd registration being synchronous (the logreader/logwriter stuff does not, as far as I understand). Other than that, it's the same as yesterday: boring stuff outside of logreader.c, logwriter.c and control.c. * "ivykis: Use github:algernon/ivykis for the time being" This changes the git submodule to point to my fork of ivkis, which has one patch over upstream: the one that removes the old-handle-related aborts from iv_fd_set_handler_{in,out,err}. A pull request has been opened[2], that was the easiest for me. This patch should be dropped once the pull request is closed. [1]: https://github.com/algernon/syslog-ng/commits/feature/3.4/ivykis/upstream [2]: https://github.com/buytenh/ivykis/pull/2
Nevertheless! The good news still is, that we're *very* close to being able to build with upstream ivykis! A few issues to fix, and then comes the next part: going over all the changes that were made to the BalaBit fork of ivykis, and send everything upstream that has not been sent yet.
And with the patches above, I'm reasonably happy with the current state of things, so it's time to move to the next item on the agenda, and diff the BalaBit fork to upstream, clean stuff up, and send patches. That will not happen overnight however, but I'll try my best to get it done faster. In parallel to that, I'll resume packaging ivykis for Debian, and start building my own syslog-ng 3.3 debs[3] against it (the packages in Debian proper will continue using the embedded fork for now). [3]: http://asylum.madhouse-project.org/projects/debian/ -- |8]
On 07/05/12 23:42, Gergely Nagy wrote:
A long long time ago, upstream and BalaBit's version of ivykis started to diverge, which - at the time - was necessary, but the intent always was to eventually get things upstream and merged, to allow syslog-ng to use upstream ivykis, as-is.
With the help of Lennert Buytenhek (ivykis upstream) and Balazs Scheidler, I present you the first work in progress patch set against syslog-ng that enables it to work with an unmodified ivykis! Hi guys,
I'm trying to follow the progress of this. Sadly, now it seems, I'm lost. Can somebody tell me the status of this WIP? Thanks, Matthias -- Matthias Runge <mrunge@matthias-runge.de> <mrunge@fedoraproject.org>
Matthias Runge <mrunge@matthias-runge.de> writes:
On 07/05/12 23:42, Gergely Nagy wrote:
A long long time ago, upstream and BalaBit's version of ivykis started to diverge, which - at the time - was necessary, but the intent always was to eventually get things upstream and merged, to allow syslog-ng to use upstream ivykis, as-is.
With the help of Lennert Buytenhek (ivykis upstream) and Balazs Scheidler, I present you the first work in progress patch set against syslog-ng that enables it to work with an unmodified ivykis! Hi guys,
I'm trying to follow the progress of this. Sadly, now it seems, I'm lost. Can somebody tell me the status of this WIP?
The syslog-ng side is, in my opinion, done, and could be merged as-is. There are a couple of patches that are on our ivykis fork that haven't been sent upstream yet. I plan to work on that backlog once I'm done with syslog-ng 3.3.6 (and had a little fun time with 3.4 stuff as a distraction). -- |8]
participants (3)
-
Gergely Nagy
-
Lennert Buytenhek
-
Matthias Runge