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 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list