[Cluster-devel] logsys in fenced

[Fabio M. Di Nitto]

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.

>  fenced-logsys.patch is my own logsys conversion for fenced.
>
> I've not compiled or tested these patches yet (the version of openais I
> have working on my test machines does not have logsys and I don't want to
> disrupt my tests on those machines.)  I'll push these once they compile.
>
> I have yet to study logsys enough to propose an alternative to the macro
> magic that's now isolated at the end of fd.h.

There are few things that don't work in this patch.

diff --git a/fence/fenced/config.c b/fence/fenced/config.c
index 56b7b0e..33fdbf4 100644
--- a/fence/fenced/config.c
+++ b/fence/fenced/config.c
@@ -1,7 +1,7 @@
  #include "fd.h"
  #include "ccs.h"

-static int open_ccs(void)
+int open_ccs(void)
  {
  	int i = 0, cd;

@@ -14,7 +14,47 @@ static int open_ccs(void)
  	return cd;
  }

^^ 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.

@@ -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?

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.

+{
+	int fd, level, loglevel, facility, y, n;
+	unsigned int logmode;
+	char name[PATH_MAX];
+	char *error; /* eh? */

This is for logsys file set. If the call fails to open a file it tells you 
why there.

+
+	fd = open_ccs();
+	if (fd < 0)
+		return fd;
+
+	/*
+	 * set level
+	 */
+
+	read_ccs_int(fd, LEVEL_PATH, &level);
+
+	loglevel = logsys_priority_id_get(level);
+	if (loglevel < 0)
+		loglevel = LOG_LEVEL_INFO;
+
+	logsys_config_priority_set(loglevel);
+
+	/*
+	 * set mode
+	 */
+
+	logmode = logsys_config_mode_get();
+
+	read_ccs_yesno(fd, "/cluster/logging/@to_stderr", &y, &n);
+	if (y)
+		logmode |= LOG_MODE_OUTPUT_STDERR;
+	if (n)
+		logmode &= ~LOG_MODE_OUTPUT_STDERR;
+
+	read_ccs_yesno(fd, "/cluster/logging/@to_syslog", &y, &n);
+	if (y)
+		logmode |= LOG_MODE_OUTPUT_SYSLOG_THREADED;
+	if (n)
+		logmode &= ~LOG_MODE_OUTPUT_SYSLOG_THREADED;
+
+	read_ccs_yesno(fd, "/cluster/logging/@to_file", &y, &n);
+	if (y)
+		logmode |= LOG_MODE_OUTPUT_FILE;
+	if (n)
+		logmode &= ~LOG_MODE_OUTPUT_FILE;
+
+	if (logmode & LOG_MODE_BUFFER_BEFORE_CONFIG) {
+		logmode &= ~LOG_MODE_BUFFER_BEFORE_CONFIG;
+		logmode |= LOG_MODE_FLUSH_AFTER_CONFIG;
+		logsys_config_mode_set(logmode);
+	}

^^^

This one will init logsys before you have finished setting it up.

+
+	/*
+	 * set log file
+	 */
+ 
+	memset(name, 0, sizeof(name));
+	read_ccs_name(fd, "/cluster/logging/@filename", name);
+
+	if (name[0])
+		logsys_config_file_set(&error, name);
+
+	/*
+	 * set facility
+	 */
+
+	memset(name, 0, sizeof(name));
+	read_ccs_name(fd, "/cluster/logging/@syslog_facility", name);
+
+	facility = logsys_facility_get(name);
+	if (facility < 0)
+		facility = SYSLOGFACILITY;
+
+	logsys_config_facility_set("FENCED", facility);
+
+	/*
+	 * set debug (wish this was yes/no like above)
+	 */
+
+	memset(name, 0, sizeof(name));
+	read_ccs_name(fd, "/cluster/logging/@debug", name);
+
+	if (!strcmp(name, "on"))
+		daemon_debug_logsys = 1;
+
+	memset(name, 0, sizeof(name));
+	read_ccs_name(fd, DEBUG_PATH, name);
+
+	if (!strcmp(name, "on"))
+		daemon_debug_logsys = 1;
+
+	close_ccs(fd);

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.

+}
+
+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.

Fabio

--
I'm going to make him an offer he can't refuse.