[Cluster-devel] logsys in fenced

[David Teigland <teigland@redhat.com>]

On Thu, Jun 26, 2008 at 05:48:56AM +0200, Fabio M. Di Nitto wrote:
> On Wed, 25 Jun 2008, David Teigland wrote:
> 
> >Attached two patches:
> > fenced-revert.patch reverts the current logsys changes to fenced.
> 
> If you revert, please do it by meaning of git-revert.

That was a series of git-revert -n which the man page suggested:

"This is useful when reverting more than one commits' effect to
 your working tree in a row."


> ^^ You can't use open_ccs within get_logsys_config_data. open_ccs uses 
> log_printf to log. If it's waiting for ccs, but logging is not available 
> yet, you end up with no information of what is going on.
> 
> In the original code, we try once to access the config data. If it works 
> good, otherwise we config logsys manually enough to log what is happening
> and we try later. You are missing this fallback mechanism.

I think I'll just avoid all the chicken/egg complication by not logging in
in open_ccs.  I expect it should be nearly impossible for ccs_connect() to
fail now with the new ccs.  (Previously, ccs_connect would always fail
leaving people wondering why nothing was happening, which is why we added
the logging.  One of the goals of the new ccs is that reading the config
should basically never fail.)


> @@ -74,14 +75,17 @@ extern void daemon_dump_save(void);
>  #define log_debug(fmt, args...) \
>  do { \
>  	snprintf(daemon_debug_buf, 255, "%ld " fmt "\n", time(NULL), 
>  	##args); \
> -	if (daemon_debug_opt) fprintf(stderr, "%s", daemon_debug_buf); \
>  	daemon_dump_save(); \
> +	if (daemon_debug_opt) \
> +		fprintf(stderr, "%s", daemon_debug_buf); \
> +	else if (daemon_debug_logsys) \
> +		log_printf(LOG_DEBUG, "%s", daemon_debug_buf); \
>  } while (0)
> 
> Can those 2 go in parallel?
> 
> +       if (daemon_debug_opt) \
> +               fprintf(stderr, "%s", daemon_debug_buf); \
> +       if (daemon_debug_logsys) \
> +               log_printf(LOG_DEBUG, "%s", daemon_debug_buf); \
> 
> so that users can decide which one they want without worrying about 
> preferences?

daemon_debug_opt and daemon_debug_logsys are mutually exclusive by
definition; you don't want or need logsys when you're running with -D.


> diff --git a/fence/fenced/logging.c b/fence/fenced/logging.c
> new file mode 100644
> index 0000000..38abc21
> --- /dev/null
> +++ b/fence/fenced/logging.c
> @@ -0,0 +1,109 @@
> +#include "fd.h"
> +
> +#define LEVEL_PATH 
> "/cluster/logging/logger_subsys[@subsys=\"FENCED\"]/@syslog_level"
> +#define DEBUG_PATH 
> "/cluster/logging/logger_subsys[@subsys=\"FENCED\"]/@debug"
> +
> +static int get_logsys_config_data(void)
> 
> Most of the configuration logic is wrong and you don't check for a bunch 
> of return codes that could at least inform you or the users of the status 
> of the configuration.

Yeah, not understanding logsys, I couldn't make sense of the logic that
was there, it was "non-obvious", so I just tried to copy it.  I obviously
have a lot of logsys study to do, hopefully suggesting some
simplifications along the way...


> Debug needs to be checked before log_level. In logsys debug=on is 
> equivalent of setting syslog_level=debug. The original code I proposed 
> does:
> 
> debug from the envvar (that i missed in my original patch) > debug from 
> cmdline > subsystem config debug option > global config debug option.
> 
> This allows you a great deal of flexibility to specify: I want all debug 
> on except for this or that subsystem, or i want debugging off, except 
> for.. etc.
> 
> Then you set log_level according to that.

that sounds like a great deal of complexity for something that shouldn't
deserve it.


> +void setup_logsys(void)
> +{
> +	get_logsys_config_data();
> +	logsys_config_mode_set(LOG_MODE_OUTPUT_STDERR |
> +			       LOG_MODE_OUTPUT_SYSLOG_THREADED |
> +			       LOG_MODE_OUTPUT_FILE |
> +			       LOG_MODE_FLUSH_AFTER_CONFIG);
> +}
> 
> this call to logsys_config_mode_set will simply override all the 
> configyration you did for STDERR SYSLOG_THREADED and FILE by setting them 
> all back on.

That puzzled me, too... ah, looks like I mistranslated the original code.