Hi, I have started merging the branch $subject, however I've found some patches I couldn't merge. system-source: Use follow-freq(1) on FreeBSD prior to 9.1 Are you sure time based polling would work here? I mean time based polling also checks if the size of the file has changed, as it was meant to be used only with regular files. I'm afraid that the inability to use kqueue on /dev/klog would prevent using kqueue completely, as the syslog-ng file source code requires either regular files or pollable devices. dbparser: support inheriting properties in a corellation action I merged this one, although I have some doubts. It uses log_msg_clone_cow(), which does have some tricky semantics, as it assumes that the cloned message will not change, once cloned. I was thinking about removing clone_cow() completely because of this tricky stuff, and whenever we need a clone we're about to change the LogMessage anyway, and delaying the actual change means that we have to do two malloc() calls, instead of one. Can you test whether changing the message that triggered the action with a rewrite rule, keeps the cloned one intact? This would need a parallel path to the db-parser(). dbparser: Make the context length available to action conditions I didn't pick this one. I was thinking about using a template function to return the number of entries in the current context. This patch changes the log message while it could keep it read-only, and even if I wanted to do this macro-like, I'd have created a real macro. Both template formatting and template functions receive the context as arguments anyway, so the required information is readily available. -- Bazsi
On Sat, Nov 17, 2012 at 9:00 AM, Balazs Scheidler <bazsi@balabit.hu> wrote:
system-source: Use follow-freq(1) on FreeBSD prior to 9.1
Are you sure time based polling would work here? I mean time based polling also checks if the size of the file has changed, as it was meant to be used only with regular files.
Yes, I am sure it works, due to this earlier patch: commit 169bc655620bf641eb0429258533037ff9ae52bd Author: Gergely Nagy <algernon@balabit.hu> Date: Fri Oct 5 12:16:57 2012 +0200 logreader: When following a file, treat non-regular files specially
I'm afraid that the inability to use kqueue on /dev/klog would prevent using kqueue completely, as the syslog-ng file source code requires either regular files or pollable devices.
With the above patch, there is a workaround that will do until we have a proper device() source. In the long run, I want a better solution than this workaround, but until then, this is in my opinion, an acceptable workaround. As the other option which does not require extensive changes is to disable kqueue completely (and fall back to poll) - which is what has been the case 'till 3.3.6, and it's just as bad as this workaround, to be honest. I'd rather not to that, since in the long run, we want device() anyway.
dbparser: support inheriting properties in a corellation action
I merged this one, although I have some doubts. It uses log_msg_clone_cow(), which does have some tricky semantics, as it assumes that the cloned message will not change, once cloned.
I was thinking about removing clone_cow() completely because of this tricky stuff, and whenever we need a clone we're about to change the LogMessage anyway, and delaying the actual change means that we have to do two malloc() calls, instead of one.
Can you test whether changing the message that triggered the action with a rewrite rule, keeps the cloned one intact? This would need a parallel path to the db-parser().
Mhm, I'll do that and report back (most likely this won't happen 'till next thursday, though).
dbparser: Make the context length available to action conditions
I didn't pick this one. I was thinking about using a template function to return the number of entries in the current context.
Indeed, using a template function crossed my mind too, but to do that, I would have to do a dance I deemed too expensive, both by the amount of code that would need to be written and the number of hoops the template function would have to jump through: it would need to be called first, then look up the context, then append the string to the result.. That seemed overly complicated to me, compared to the few lines the patch adds.
This patch changes the log message while it could keep it read-only, and even if I wanted to do this macro-like, I'd have created a real macro. Both template formatting and template functions receive the context as arguments anyway, so the required information is readily available.
On the other hand, yeah... keeping the message read-only would be a nice bonus. I'll rework this patch. -- |8]
----- Original message -----
On Sat, Nov 17, 2012 at 9:00 AM, Balazs Scheidler <bazsi@balabit.hu> wrote:
system-source: Use follow-freq(1) on FreeBSD prior to 9.1
Are you sure time based polling would work here? I mean time based polling also checks if the size of the file has changed, as it was meant to be used only with regular files.
Yes, I am sure it works, due to this earlier patch:
commit 169bc655620bf641eb0429258533037ff9ae52bd Author: Gergely Nagy <algernon@balabit.hu> Date: Fri Oct 5 12:16:57 2012 +0200
logreader: When following a file, treat non-regular files specially
I'm afraid that the inability to use kqueue on /dev/klog would prevent using kqueue completely, as the syslog-ng file source code requires either regular files or pollable devices.
With the above patch, there is a workaround that will do until we have a proper device() source.
you mean, that with that workaround, logreader will unconditionally read from the fd every follow-freq() seconds, assuming there's something to be read? I don't see how the device() driver would solve this though. In the long run, I want a better solution
than this workaround, but until then, this is in my opinion, an acceptable workaround. As the other option which does not require extensive changes is to disable kqueue completely (and fall back to poll) - which is what has been the case 'till 3.3.6, and it's just as bad as this workaround, to be honest. I'd rather not to that, since in the long run, we want device() anyway.
agaimn, I don't see how you'd solve this case properly. w/o kqueue support on /dev/log, we'll have to resort to timer based polling.
dbparser: support inheriting properties in a corellation action
I merged this one, although I have some doubts. It uses log_msg_clone_cow(), which does have some tricky semantics, as it assumes that the cloned message will not change, once cloned.
I was thinking about removing clone_cow() completely because of this tricky stuff, and whenever we need a clone we're about to change the LogMessage anyway, and delaying the actual change means that we have to do two malloc() calls, instead of one.
Can you test whether changing the message that triggered the action with a rewrite rule, keeps the cloned one intact? This would need a parallel path to the db-parser().
Mhm, I'll do that and report back (most likely this won't happen 'till next thursday, though).
I've travelled into the countryside for the weekend, so probably I won't be checking into this either.
dbparser: Make the context length available to action conditions
I didn't pick this one. I was thinking about using a template function to return the number of entries in the current context.
Indeed, using a template function crossed my mind too, but to do that, I would have to do a dance I deemed too expensive, both by the amount of code that would need to be written and the number of hoops the template function would have to jump through: it would need to be called first, then look up the context, then append the string to the result.. That seemed overly complicated to me, compared to the few lines the patch adds.
You don't have to look up anything, the context is passed to template functions through the invoke args stuff. There are already template functions that use the context, see for instance $(grep)
This patch changes the log message while it could keep it read-only, and even if I wanted to do this macro-like, I'd have created a real macro. Both template formatting and template functions receive the context as arguments anyway, so the required information is readily available.
On the other hand, yeah... keeping the message read-only would be a nice bonus. I'll rework this patch.
Balazs Scheidler <bazsi77@gmail.com> writes:
commit 169bc655620bf641eb0429258533037ff9ae52bd Author: Gergely Nagy <algernon@balabit.hu> Date: Fri Oct 5 12:16:57 2012 +0200
logreader: When following a file, treat non-regular files specially
I'm afraid that the inability to use kqueue on /dev/klog would prevent using kqueue completely, as the syslog-ng file source code requires either regular files or pollable devices.
With the above patch, there is a workaround that will do until we have a proper device() source.
you mean, that with that workaround, logreader will unconditionally read from the fd every follow-freq() seconds, assuming there's something to be read?
Pretty much, yes, provided the fd shows certain properties which I forgot (I believe it was st.st_size being 0).
I don't see how the device() driver would solve this though.
It would 'solve' it by not having to have this hack in logreader, but in a device-specific driver, a few layers elsewhere.
agaimn, I don't see how you'd solve this case properly. w/o kqueue support on /dev/log, we'll have to resort to timer based polling.
Yes, timer based polling is the good solution, and one reason device() would solve it, is that it wouldn't have follow-freq(0). The reason /dev/klog did not work is because we fell back to timer-based polling, but that is disabled with follow-freq(0). With device(), we can get rid of that without resorting to crude hackery.
You don't have to look up anything, the context is passed to template functions through the invoke args stuff. There are already template functions that use the context, see for instance $(grep)
Hrm... I didn't look hard enough then. I'll take a look at $(grep) and rework the patch, thanks! -- |8]
Balazs Scheidler <bazsi77@gmail.com> writes:
dbparser: Make the context length available to action conditions
I didn't pick this one. I was thinking about using a template function to return the number of entries in the current context.
Indeed, using a template function crossed my mind too, but to do that, I would have to do a dance I deemed too expensive, both by the amount of code that would need to be written and the number of hoops the template function would have to jump through: it would need to be called first, then look up the context, then append the string to the result.. That seemed overly complicated to me, compared to the few lines the patch adds.
You don't have to look up anything, the context is passed to template functions through the invoke args stuff. There are already template functions that use the context, see for instance $(grep)
I've implemented $(context-length) now, it's pushed to the merge-queue/3.4 branch. It was easier than I thought, and more straightforward than the previous $CONTEXT_LENGTH thing, too. -- |8]
On 11/17/2012 09:00 AM, Balazs Scheidler wrote:
dbparser: support inheriting properties in a corellation action
I merged this one, although I have some doubts. It uses log_msg_clone_cow(), which does have some tricky semantics, as it assumes that the cloned message will not change, once cloned.
I was thinking about removing clone_cow() completely because of this tricky stuff, and whenever we need a clone we're about to change the LogMessage anyway, and delaying the actual change means that we have to do two malloc() calls, instead of one.
Can you test whether changing the message that triggered the action with a rewrite rule, keeps the cloned one intact? This would need a parallel path to the db-parser(). It seems, that log_msg_clone_cow write protects the original log message, so trying to do a log_msg_set_value on the original message will result in an assertion. Sorry I didn't realize this when doing the original patch, explicitly copying all the name-value pairs seemed inefficient, but this behaviour is not desirable. Should I rework the patch this way or is there a better way of doing it?
Balint
----- Original message -----
On 11/17/2012 09:00 AM, Balazs Scheidler wrote:
dbparser: support inheriting properties in a corellation action
I merged this one, although I have some doubts. It uses log_msg_clone_cow(), which does have some tricky semantics, as it assumes that the cloned message will not change, once cloned.
I was thinking about removing clone_cow() completely because of this tricky stuff, and whenever we need a clone we're about to change the LogMessage anyway, and delaying the actual change means that we have to do two malloc() calls, instead of one.
Can you test whether changing the message that triggered the action with a rewrite rule, keeps the cloned one intact? This would need a parallel path to the db-parser(). It seems, that log_msg_clone_cow write protects the original log message, so trying to do a log_msg_set_value on the original message will result in an assertion. Sorry I didn't realize this when doing the original patch, explicitly copying all the name-value pairs seemed inefficient, but this behaviour is not desirable. Should I rework the patch this way or is there a better way of doing it?
I'd drop clone_cow() now, and implement a proper clone that allocates the logmsg header and nvtable in a single block, and copies everything through. that way clone+change becomes cheaper and you got a clone call that works in this case too.
Balazs Scheidler <bazsi77@gmail.com> writes:
----- Original message -----
On 11/17/2012 09:00 AM, Balazs Scheidler wrote:
dbparser: support inheriting properties in a corellation action
I merged this one, although I have some doubts. It uses log_msg_clone_cow(), which does have some tricky semantics, as it assumes that the cloned message will not change, once cloned.
I was thinking about removing clone_cow() completely because of this tricky stuff, and whenever we need a clone we're about to change the LogMessage anyway, and delaying the actual change means that we have to do two malloc() calls, instead of one.
Can you test whether changing the message that triggered the action with a rewrite rule, keeps the cloned one intact? This would need a parallel path to the db-parser(). It seems, that log_msg_clone_cow write protects the original log message, so trying to do a log_msg_set_value on the original message will result in an assertion. Sorry I didn't realize this when doing the original patch, explicitly copying all the name-value pairs seemed inefficient, but this behaviour is not desirable. Should I rework the patch this way or is there a better way of doing it?
I'd drop clone_cow() now, and implement a proper clone that allocates the logmsg header and nvtable in a single block, and copies everything through.
that way clone+change becomes cheaper and you got a clone call that works in this case too.
There's another problem, though: pattern_db_process() also sets the original message write protected, so even if I do a proper clone, the original will remain write protected. I'm also a bit confused about the single block header + nvtable allocation... I suppose I can do clone = log_msg_alloc(msg->payload->size), copy the header and the payload too and adjust clone->payload. Is that the correct way to do it? -- |8]
----- Original message -----
Balazs Scheidler <bazsi77@gmail.com> writes:
----- Original message -----
On 11/17/2012 09:00 AM, Balazs Scheidler wrote:
dbparser: support inheriting properties in a corellation action
I merged this one, although I have some doubts. It uses log_msg_clone_cow(), which does have some tricky semantics, as it assumes that the cloned message will not change, once cloned.
I was thinking about removing clone_cow() completely because of this tricky stuff, and whenever we need a clone we're about to change the LogMessage anyway, and delaying the actual change means that we have to do two malloc() calls, instead of one.
Can you test whether changing the message that triggered the action with a rewrite rule, keeps the cloned one intact? This would need a parallel path to the db-parser(). It seems, that log_msg_clone_cow write protects the original log message, so trying to do a log_msg_set_value on the original message will result in an assertion. Sorry I didn't realize this when doing the original patch, explicitly copying all the name-value pairs seemed inefficient, but this behaviour is not desirable. Should I rework the patch this way or is there a better way of doing it?
I'd drop clone_cow() now, and implement a proper clone that allocates the logmsg header and nvtable in a single block, and copies everything through.
that way clone+change becomes cheaper and you got a clone call that works in this case too.
There's another problem, though: pattern_db_process() also sets the original message write protected, so even if I do a proper clone, the original will remain write protected.
only in case it's stored in a correllation context.
I'm also a bit confused about the single block header + nvtable allocation...
I suppose I can do clone = log_msg_alloc(msg->payload->size), copy the header and the payload too and adjust clone->payload. Is that the correct way to do it?
I can't really remember the code too much. double check whether there are pointer values, those would probably need adjustments. also there are iv_list_heads used when a messagee is enqueued that need cleaning.
participants (5)
-
Balazs Scheidler
-
Balazs Scheidler
-
Balint Kovacs
-
Gergely Nagy
-
Gergely Nagy