10:21 a.m.
On Wed, Dec 17, 2008 at 04:12:20PM +0100, Daniel Veillard wrote:
diff -u -r1.1 logging.h
--- src/logging.h 6 Nov 2008 16:36:07 -0000 1.1
+++ src/logging.h 17 Dec 2008 14:25:57 -0000
@@ -30,16 +30,87 @@
* defined at runtime of from the libvirt daemon configuration file
*/
#ifdef ENABLE_DEBUG
-extern int debugFlag;
#define VIR_DEBUG(category, fmt,...) \
- do { if (debugFlag) fprintf (stderr, "DEBUG: %s: %s (" fmt
")\n", category, __func__, __VA_ARGS__); } while (0)
+ virLogMessage(category, VIR_LOG_DEBUG, 0, fmt, __VA_ARGS__)
+#define VIR_INFO(category, fmt,...) \
+ virLogMessage(category, VIR_LOG_INFO, 0, fmt, __VA_ARGS__)
+#define VIR_WARN(category, fmt,...) \
+ virLogMessage(category, VIR_LOG_WARN, 0, fmt, __VA_ARGS__)
+#define VIR_ERROR(category, fmt,...) \
+ virLogMessage(category, VIR_LOG_ERROR, 0, fmt, __VA_ARGS__)
#else
#define VIR_DEBUG(category, fmt,...) \
do { } while (0)
+#define VIR_INFO(category, fmt,...) \
+ do { } while (0)
+#define VIR_WARN(category, fmt,...) \
+ do { } while (0)
+#define VIR_ERROR(category, fmt,...) \
+ do { } while (0)
#endif /* !ENABLE_DEBUG */
#define DEBUG(fmt,...) VIR_DEBUG(__FILE__, fmt, __VA_ARGS__)
#define DEBUG0(msg) VIR_DEBUG(__FILE__, "%s", msg)
+#define INFO(fmt,...) VIR_INFO(__FILE__, fmt, __VA_ARGS__)
+#define INFO0(msg) VIR_INFO(__FILE__, "%s", msg)
+#define WARN(fmt,...) VIR_WARN(__FILE__, fmt, __VA_ARGS__)
+#define WARN0(msg) VIR_WARN(__FILE__, "%s", msg)
+#define ERROR(fmt,...) VIR_ERROR(__FILE__, fmt, __VA_ARGS__)
+#define ERROR0(msg) VIR_ERROR(__FILE__, "%s", msg)
I think we should add a prefix to the filename to give a
little scope to the namespace. This would let people use
non-filename based namespaces without risk of clashing,
eg, perhaps
#define ERROR(fmt,...) VIR_ERROR("file." __FILE__, fmt, __VA_ARGS__)
thus turning into file.libvirt.c
+/**
+ * virLogOutputFunc:
+ * @data: extra output logging data
+ * @category: the category for the message
+ * @priority: the priority for the message
+ * @msg: the message to log, preformatted and zero terminated
+ * @len: the lenght of the message in bytes without the terminating zero
+ *
+ * Callback function used to output messages
+ *
+ * Returns the number of bytes written or -1 in case of error
+ */
+typedef int (*virLogOutputFunc) (void *data, const char *category,
+ int priority, const char *str, int len);
I have a preference for 'void *data' parameters to callbacks being
at end of the param list.
+extern int virLogStartup(void);
+extern int virLogReset(void);
+extern void virLogShutdown(void);
+extern int virLogParseFilters(const char *filters);
+extern int virLogParseOutputs(const char *output);
+extern void virLogMessage(const char *category, int priority, int flags,
+ const char *fmt, ...) ATTRIBUTE_FORMAT(printf, 4, 5);
I think it would be a good idea to have virLogMesage take a
function name, and line number too. Likewise for the
virLogOutputFunc() function.
The VIR_ERROR/WARN/INFO/DEBUG macros would automatically include
the __func__ and __line__ macros. eg
#define VIR_INFO(category, fmt,...) \
virLogMessage(category, __func__, __line__, VIR_LOG_INFO, 0, fmt,
__VA_ARGS__)
So when we process the 'char fmt,....' into an actual string we could
have some flag to turn on/off inclusion of the function & line data.
Cole did a similar thing for error reporting internal APIs when he
added this to virterror_internal.h:
void virReportErrorHelper(virConnectPtr conn, int domcode, int errcode,
const char *filename,
const char *funcname,
long long linenr,
const char *fmt, ...)
ATTRIBUTE_FORMAT(printf, 7, 8);
Though, the actual implementation currently ignores filename, funcname
and linenr.
+
+/*
+ * Macro used to format the message as a string in virLogMessage
+ * and borrowed from libxml2 (also used in virRaiseError)
+ */
+#define VIR_GET_VAR_STR(msg, str) { \
+ int size, prev_size = -1; \
+ int chars; \
+ char *larger; \
+ va_list ap; \
+ \
+ str = (char *) malloc(150); \
+ if (str != NULL) { \
+ \
+ size = 150; \
+ \
+ while (1) { \
+ va_start(ap, msg); \
+ chars = vsnprintf(str, size, msg, ap); \
+ va_end(ap); \
+ if ((chars > -1) && (chars < size)) { \
+ if (prev_size == chars) { \
+ break; \
+ } else { \
+ prev_size = chars; \
+ } \
+ } \
+ if (chars > -1) \
+ size += chars + 1; \
+ else \
+ size += 100; \
+ if ((larger = (char *) realloc(str, size)) == NULL) { \
+ break; \
+ } \
+ str = larger; \
+ }} \
+}
Wonder if we should make this use virBuffer instead of
char/malloc/realloc. Could then add this definition to
buf.h and share it between the logging & error routines.
+static const char *virLogPriorityString(virLogPriority lvl) {
+ switch (lvl) {
+ case VIR_LOG_DEBUG:
+ return("debug");
+ case VIR_LOG_INFO:
+ return("info");
+ case VIR_LOG_WARN:
+ return("warning");
+ case VIR_LOG_ERROR:
+ return("error");
+ }
+ return("unknown");
+}
+
+static int virLogInitialized = 0;
+
+/**
+ * virLogStartup:
+ *
+ * Initialize the logging module
+ *
+ * Returns 0 if successful, and -1 in case or error
+ */
+int virLogStartup(void) {
+ if (virLogInitialized)
+ return(-1);
+ virLogInitialized = 1;
Strictly speaking we should use pthread_once() for such
initializations, or an atomic test-and-set operation.
+ /*
+ * serialize the error message, add level and timestamp
+ */
+ VIR_GET_VAR_STR(fmt, str);
+ if (str == NULL)
+ return;
+ gettimeofday(&cur_time, NULL);
+ localtime_r(&cur_time.tv_sec, &time_info);
+
+ if (asprintf(&msg, "%02d:%02d:%02d.%03d: %s : %s\n",
+ time_info.tm_hour, time_info.tm_min,
+ time_info.tm_sec, (int) cur_time.tv_usec / 1000,
+ virLogPriorityString(priority), str) < 0) {
This would be the place where we'd add in (optional) inclusion of
the function name & line number data.
+
+#define IS_SPACE(cur) \
+ ((*cur == ' ') || (*cur == '\t') || (*cur == '\n') ||
\
+ (*cur == '\r') || (*cur == '\\'))
GNULIB already provides this with c_isspace() - unless we really
need the check for '\\' too ?
All basically looks good to me.
Regards,
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|