[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [Cluster-devel] logsys in fenced



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.


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]