loggen: fix crash with invalid parameterization

[furiel]

../bin/loggen -S --active-connections=10 -I 120 caused crash in
loggen, because mandatory parameters were not passed: ip and port.

The reason is even if a module cannot start, stop was still called. In
our specific case, thread_array was accessed in stop, even though it
was not initialized in start (because start returned earlier due to error).

The fix is that start returns boolean now. Stop is only called a
plugin is started.

[kira-syslogng]

Build FAILURE

[furiel]

@kira-syslogng retest this please

[kira-syslogng]

Build SUCCESS

[norberttak]

In general I like these changes. I know that we (currently) allow only one plugin to be active at a time so your changes are correct. If in the future we will allow to run more than one plugin your change might cause plugins not stopped if other plugins were not started successfully.

So I suggest to prepare the plugin's stop function to handle the situation when a plugin was not activated and call all plugin's stop function every time.

As I see the crash caused by the missing thread_array so I simple check for this can avoid the crash in the stop function

[gaborznagy]

I like this approach better with explicitly stating that 0 plugin started.
In case of multiple plugins will be needed, this will be modified.

[furiel]

@norberttak I was thinking about the alternative of making the stop non-start safe. In the end I opted for not calling stop when a plugin was not started because I felt that the other version would induce some implementation requirements/architectural complexity that we are not used to, or inconvenient.

For example:

• Conceptually, it feels odd that we can stop something that is not started.
• Start can fail. And for different reasons. It can be an argparse error. Or connectivity error.
  Currently, besides responsible for initialization, start is also responsible for running. (Later we might want to split into init and run functions). Failure can happen unexpectedly during transmit: for example when a remote stops unexpectedly.
  Expressing that start cannot fail (it is a void function with no error propagation), also feels odd.
• Start will partitally initialize the plugin in case of failure. It is more error prone to prepare the stop function to stop partially initialized objects (like this bug).

If in the future we will allow to run more than one plugin

It may, but I have a little doubts. At least not in the near future. For any usecase that I can think of, we can always start more loggen instances. On the other hand, having a controller node over multiple active plugins has it's own challenges.

• What would happen if some plugins cannot start? Do we restart them? Should loggen start at all? Do we cancel those that are started? (supervisor-like functionality)
• How do we configure? Specifying multiple plugins on the command line with same options is difficult, because the same option needs to become context aware: for example I might want a tcp destination with one rate limit, and an udp with another. Will we plan to have a configuration language?
• What if I want to use the same plugin multiple times? (Partially the previous queston).

Technically all these can be implemented with some effort. But as most of these questions can be avoided by executing multiple loggen instances in parallel, I think the implementation will not happen in the near future.

your change might cause plugins not stopped if other plugins were not started successfully.

After my change, start_plugins will return with zero, so the code quickly returns to the main function and the program exits immediately. This kind of imitates my preference: loggen should not start if one of the plugins cannot start. This can be challenged of course (or make configurable?), when we implement the multiple active plugin version. But for now I do not see how this can be imlemented, so I would like to avoid preparing for that.

[norberttak]

accept your opinion.
I don't want to introduce more complexity in the loggen. I just want to hint that the original issue (the thread_array variable has not allocated in the stop function in case of error) has not fixed. I have a filing that stop function shall check the existence of thread_array similar to "PluginOption *option" variable.
Let's consider my finding as an optional review finding, I'm fine with the current form of the code.